Fix invalid projection in CommonSubexprEliminate
#2915
Conversation
|
@jdye64 Here is the fix for the root cause of the invalid projections in the optimizer |
|
@waynexia fyi |
Codecov Report
@@ Coverage Diff @@
## master #2915 +/- ##
=======================================
Coverage 85.30% 85.31%
=======================================
Files 274 274
Lines 49267 49279 +12
=======================================
+ Hits 42028 42040 +12
Misses 7239 7239
Continue to review full report at Codecov.
|
| @@ -282,15 +282,13 @@ fn build_project_plan( | |||
| } | |||
|
|
|||
| for field in input.schema().fields() { | |||
| if !fields_set.contains(field.name()) { | |||
| fields_set.insert(field.name().to_owned()); | |||
| if fields_set.insert(field.qualified_name()) { | |||
There was a problem hiding this comment.
Nice simplification
Use qualified name instead of unqualified name in the HashSet that tracks which fields have already been added to the projection
This is correct 👍 It used to be inconsistent with L287 which uses qualified name.
| fields.push(field.clone()); | ||
| project_exprs.push(Expr::Column(field.qualified_column())); | ||
| } | ||
| } | ||
|
|
||
| let mut schema = DFSchema::new_with_metadata(fields, HashMap::new())?; | ||
| schema.merge(input.schema()); |
There was a problem hiding this comment.
merge is done in the previous for loop so I think this is redundant. Let's remove it.
|
Benchmark runs are scheduled for baseline = d8a2f58 and contender = b5537e7. b5537e7 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 #2907
Rationale for this change
CommonSubexprEliminatecreated invalid projections when processing joins where there were one or more columns with the same unqualified name on each side of the join (as happens when you join to the same table multiple times).What changes are included in this PR?
HashSetthat tracks which fields have already been added to the projectionAre there any user-facing changes?
No