Skip to content

Conversation

@huaxingao
Copy link
Contributor

What changes were proposed in this pull request?

backporting #39152 to 3.3

Why are the changes needed?

bug fixing

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

@github-actions github-actions bot added the SQL label Apr 21, 2023
@huaxingao
Copy link
Contributor Author

node
} else {
val newNode = addMetadataCol(node)
val newNode = node.mapChildren(addMetadataCol(_, metaCols.map(_.exprId).toSet))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this only does addMetadataCol on children?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is from this fix #39895

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me because we need to propagate metadata cols to this node and need to add metadata cols in its children. Does master branch have the same code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, master branch has the same code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there was follow up.

Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! If anyone else was seeing odd issues with mispropagated metadata columns this should fix for them as well.

huaxingao added a commit that referenced this pull request Apr 21, 2023
### What changes were proposed in this pull request?
backporting #39152 to 3.3

### Why are the changes needed?
bug fixing

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
UT

Closes #40889 from huaxingao/metadata.

Authored-by: huaxingao <[email protected]>
Signed-off-by: huaxingao <[email protected]>
@huaxingao
Copy link
Contributor Author

Merged to branch-3.3. Thank you all for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants