fix: When consuming Substrait, temporarily rename clashing duplicate columns #11329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alamb
approved these changes
Jul 8, 2024
| new_name = format!("{}__temp__{}", name, i); | ||
| i += 1; | ||
| } | ||
| names.insert(new_name.clone()); |
There was a problem hiding this comment.
You could probably save a clone by putting the names.insert call below the exprs.push calls
There was a problem hiding this comment.
I made this change in #11337 as I had the code open anyways
| if new_name != name { | ||
| exprs.push(x.as_ref().clone().alias(new_name.clone())); | ||
| } else { | ||
| exprs.push(x.as_ref().clone()); |
There was a problem hiding this comment.
I was confused at first why this was as_ref().clone() was needed
When I investigated, it seems that it is due to the fact that from_substrait_rex returns an Arc<Expr> rather than an Expr which is somewhat confusing to me
There was a problem hiding this comment.
Turns out I can remove them: #11337
findepi
pushed a commit
to findepi/datafusion
that referenced
this pull request
Jul 16, 2024
…columns (apache#11329) * cleanup project internals * alias intermediate duplicate columns * fix test * fix clippy
xinlifoobar
pushed a commit
to xinlifoobar/datafusion
that referenced
this pull request
Jul 18, 2024
…columns (apache#11329) * cleanup project internals * alias intermediate duplicate columns * fix test * fix clippy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Related to #10815, and follows up from #11049 (comment). There may still be some cases left uncovered, but this does handle some more.
Closes #.
Rationale for this change
As DF requires column name uniqueness while Substrait doesn't care about names at all (apart from first & last levels of the plan), that causes clashes in the intermediate nodes that prevent DF from executing plans.
A simple example of a plan that would not work is:
This fails as Substrait consumer first creates a project to add the columns, and only then creates the final project (or modifies the existing project) to rename them. But when the first project is created, it has two identical expressions without aliases, meaning their names are collide and
validate_unique_names()fails.What changes are included in this PR?
When consuming Substrait
Projects, rename expressions as required to prevent duplicates.Are these changes tested?
Added unit test + existing tests
Are there any user-facing changes?