-
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
Add config option for coalesce_batches physical optimization rule, make optional #2791
Conversation
datafusion/core/src/config.rs
Outdated
for config in configs | ||
.config_definitions | ||
.iter() | ||
.sorted_by_key(|c| c.key.clone()) |
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.
Make the output deterministic
Codecov Report
@@ Coverage Diff @@
## master #2791 +/- ##
==========================================
+ Coverage 85.14% 85.22% +0.08%
==========================================
Files 273 274 +1
Lines 48248 48621 +373
==========================================
+ Hits 41079 41436 +357
- Misses 7169 7185 +16
Continue to review full report at Codecov.
|
@@ -27,6 +28,13 @@ pub const OPT_FILTER_NULL_JOIN_KEYS: &str = "datafusion.optimizer.filter_null_jo | |||
/// Configuration option "datafusion.execution.batch_size" | |||
pub const OPT_BATCH_SIZE: &str = "datafusion.execution.batch_size"; | |||
|
|||
/// Configuration option "datafusion.execution.coalesce_batches" |
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 really like how this new option framework is coming together ❤️
Arc::new(AggregateStatistics::new()), | ||
Arc::new(HashBuildProbeOrder::new()), | ||
]; | ||
if config.config_options.get_bool(OPT_COALESCE_BATCHES) { |
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 wonder if it would be helpful to add a test to ensure the option to disable coalsce'ing batches doesn't get broken in the future
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.
Sounds good. I will get to this in the next day or two.
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 have added tests for disabling the optimizer rule and also for setting a custom batch size.
Which issue does this PR close?
Closes #2790
Rationale for this change
Give users control over optimization rules that are applied.
What changes are included in this PR?
Are there any user-facing changes?
No