Do not error in optimizer if resulting schema has different metadata #4347
Conversation
| fn generate_same_schema_different_metadata() { | ||
| // if the plan creates more metadata than previously (because | ||
| // some wrapping functions are removed, etc) do not error | ||
| let opt = Optimizer::with_rules(vec![Arc::new(GetTableScanRule {})]); |
There was a problem hiding this comment.
this test mimics what I was seeing in IOx
| @@ -225,7 +225,7 @@ impl Optimizer { | |||
| let result = rule.try_optimize(&new_plan, optimizer_config); | |||
| match result { | |||
| Ok(Some(plan)) => { | |||
| if plan.schema() != new_plan.schema() { | |||
| if !plan.schema().equivalent_names_and_types(new_plan.schema()) { | |||
There was a problem hiding this comment.
This is the actual change
alamb
force-pushed
the
alamb/fix_schema_error
branch
from
f7c5e93
to
ca48b22
Compare
|
@jackwener and @mingmwang can you please take a look? |
alamb
force-pushed
the
alamb/fix_schema_error
branch
from
210469f
to
65e2b3e
Compare
| && f1.name() == f2.name() | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Some complex arrow types are nested types and can include other Fields.
/// A list of some logical data type with variable length.
List(Box<Field>),
/// A list of some logical data type with fixed length.
FixedSizeList(Box<Field>, i32),
/// A list of some logical data type with variable length and 64-bit offsets.
LargeList(Box<Field>),
/// A nested datatype that contains a number of sub-fields.
Struct(Vec<Field>),
|
I would suggest to add the schema equal method to arrow-rs add the new method for |
This is a good idea -- I will do so |
|
As this error is blocking my upgrade of DataFusion into IOx my plan is to file tickets for the follow on work and merge this one in (I will also try and prepare a PR to support the nested types as well) |
|
Follow up in arrow-rs: apache/arrow-rs#3199 (will add this link in comments in a follow on PR) |
|
Benchmark runs are scheduled for baseline = ba73c81 and contender = a61615b. a61615b 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 #4346
Note this PR is almost all tests
Rationale for this change
Optimizer passes do not necessarily keep nullability and metadata the same between passes
See #4346 for more detail
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No