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.
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
[SPARK-34527][SQL] Resolve duplicated common columns from USING/NATURAL JOIN #31666
[SPARK-34527][SQL] Resolve duplicated common columns from USING/NATURAL JOIN #31666
Changes from 6 commits
1c5ab03
2fe733f
2c261bb
80beda8
e1719d3
6fa70ba
0ba1916
2b7e730
0c116a5
bf87f55
b5dc44f
6e32b3d
7c3f5df
181751a
ad5e824
e36e853
db44c53
73b7c8a
1eb01e2
9fd2490
f5cc3ae
fa7207e
7af12ae
07f9ad5
c474745
0f267e7
ed0270c
66ad572
fc3b16d
f665030
85b81b1
eab7964
44ee9f8
c84f396
0fe04a2
c7c3df6
b1bf28d
47be66d
8c5144e
333a815
49de5c5
9e62d7d
446d4bc
8f70c2d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: we can avoid building a new
Seq
frequently. The check can beplan.children.exists(c => c.metadataOutput.exists(_.exprId == a.exprId))
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.
The same to
hasMetadataCol
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.
This occurs in the case that a column is resolved below the level at which it becomes labeled as metadata. For the NATURAL/USING JOIN, this occurs when the column is resolved at the level of the root table - it is only labeled as hidden when it is used as a key column in the join.
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.
why do we move this method? It creates a lot of code diff and makes it harder to review.
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 can move it back - I just wasn't sure why it lived outside of this class, given that it's not shared.
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.
This exists in
transformUp
, but not inresolveOperatorsUp
- was the difference intentional or unintentional? Without the tags, the metadata cannot be resolved properly (isMetadataCol
is always false).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 think it's a mistake.
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.
It's unfortunate that we need to use
TreeNodeTag
to store the extra information inProject
, but I don't have a better idea without changing theProject
constructor.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.
We could make this more generic by adding this
LogicalPlan
'smetadataOutput
, but that would complicate how we can add these hidden columns inAddMetadataColumns
.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.
The semantic is clear now, let's refine the naming.
We only have metadata column, and metadata column can be included in qualified star if required. We can just add a new property to metadata columns to indicate it.
The property name can be
__support_qualified_star
, and the helper class can be