-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove tests from sql_integration that were ported to sqllogictest #4836
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added sqllogicaltest does not cover all the removed tests.
Thanks @matthewwillian and @jackwener -- I plan to review this PR tomorrow as well. |
Thanks @jackwener. One of the aggregates tests and all of the arrow_typeof tests are duplicates of tests that already exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @matthewwillian -- this looks great to me. I double checked the tests and 👍
Deleting code is a wonderful first contribution 🎉
@@ -20,24 +20,6 @@ use datafusion::scalar::ScalarValue; | |||
use datafusion::test_util::scan_empty; | |||
use datafusion_common::cast::as_float64_array; | |||
|
|||
#[tokio::test] | |||
async fn csv_query_avg_multi_batch() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -846,26 +846,23 @@ SELECT count(1 + 1) | |||
---- | |||
1 | |||
|
|||
# FIX: "CSV Writer does not support List(Field { name: \"item\", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: None }) data type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -976,6 +973,7 @@ select max(c1) from d_table | |||
---- | |||
110.009 | |||
|
|||
# FIX: doesn't check datatype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tracked by #4499
} | ||
|
||
#[tokio::test] | ||
async fn arrow_typeof_boolean() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked -- here is the corresponding .slt file:
It appears there are some CI checks failing on this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jackwener. One of the aggregates tests and all of the arrow_typeof tests are duplicates of tests that already exist.
Looks great to me, cleanup the useless code is great to me.
I have checked the tests. Thanks @matthewwillian.
Benchmark runs are scheduled for baseline = c4f4dff and contender = 42f7dd5. 42f7dd5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4498
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Unit-tests pass
Are there any user-facing changes?
No