Move ConfigOptions to core #4803
Conversation
github-actions
bot
added
core
datafusion/common/Cargo.toml
Outdated
| @@ -47,3 +47,4 @@ object_store = { version = "0.5.0", default-features = false, optional = true } | |||
| parquet = { version = "29.0.0", default-features = false, optional = true } | |||
| pyo3 = { version = "0.17.1", optional = true } | |||
| sqlparser = "0.29" | |||
| num_cpus = "1.13.0" | |||
There was a problem hiding this comment.
This is the only slight downside, but I'm inclined to think this isn't a big deal
tustvold
force-pushed
the
move-config-to-common
branch
from
ecd33fa
to
85aab58
Compare
tustvold
force-pushed
the
move-config-to-common
branch
from
85aab58
to
f852074
Compare
| @@ -92,9 +92,12 @@ pub struct OptimizerContext { | |||
| impl OptimizerContext { | |||
| /// Create optimizer config | |||
| pub fn new() -> Self { | |||
| let mut options = ConfigOptions::default(); | |||
| options.optimizer.filter_null_join_keys = true; | |||
There was a problem hiding this comment.
I double-checked this wasn't introduced by the recent config rework, and confirmed that it has always been the case that OptimizerContext defaults this to true, but ConfigOptions defaults it to false.
There was a problem hiding this comment.
My intention was always to have this default to false so this looks like a bug
There was a problem hiding this comment.
OptimizerContext is now only used within tests, and so I am going to leave this for now. If you feel strongly I can file a follow up PR to change this, but it creates a non-trivial amount of test-churn
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| //! DataFusion Configuration Options | |||
|
|
|||
| use datafusion_common::{DataFusionError, Result}; | |||
| use crate::{DataFusionError, Result}; | |||
| ) -> Result<Option<LogicalPlan>> { | ||
| if !config.options().optimizer.filter_null_join_keys { | ||
| return Ok(None); |
There was a problem hiding this comment.
maybe it is worth a debug! here if this is skipped
| @@ -91,28 +85,25 @@ pub struct OptimizerContext { | |||
| /// Query execution start time that can be used to rewrite | |||
| /// expressions such as `now()` to use a literal value instead | |||
| query_execution_start_time: DateTime<Utc>, | |||
|
Benchmark runs are scheduled for baseline = ae1465d and contender = e1dc962. e1dc962 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?
Part of #4349
Rationale for this change
Moves
ConfigOptionstocommonso it can be used directly by the optimizer, planner, etc... greatly simplifying plumbing configuration around the placeWhat changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?