-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Bug][Datafusion] fix TaskContext session_config bug #2070
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.
LGTM
.await?; | ||
|
||
// test udf | ||
let df = ctx.sql("select count(1) from aggregate_test_100").await?; |
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.
💯 for test
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.
@alamb
Do we have the UT or IT for the ballista side?
I think this test_sql.rs
file just adds an example for the ballista.
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.
Maybe we can add this test for the UT or it IT framework.
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.
@liukun4515 I think we need a ballista-integration-test cargo.
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.
@liukun4515 How about add a ballista-integration-test cargo?
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 think https://github.com/apache/arrow-datafusion/issues/321 tracks some of this -- I will make a note
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.
about add a ballista-integration-
agree with the suggestion.
we can discuss this in this issue https://github.com/apache/arrow-datafusion/issues/321.
@gaojun2048 you can provide some suggestions or proposals.
You can point out some lack of ballista about test and record them.
We can make a plan to address them in Q2.
session_config | ||
} else { | ||
session_config | ||
.with_batch_size(props.get(BATCH_SIZE).unwrap().parse().unwrap()) |
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.
FYI @mingmwang
My bad. The properties in the TaskContext is not populated in this PR. The fix LGTM. |
Thanks @gaojun2048 |
closes #2069