Skip to content
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

Perform explicit UnsafeRow projection in ColumnarToRow transition #5274

Merged
merged 6 commits into from
Apr 25, 2022

Conversation

jlowe
Copy link
Member

@jlowe jlowe commented Apr 18, 2022

Fixes #5189.

The row to columnar transition was assuming that every row coming in was an UnsafeRow but that is not always the case. This adds an explicit unsafe projection to the generated unsafe row iterator to handle those cases.

Signed-off-by: Jason Lowe <jlowe@nvidia.com>
@jlowe jlowe self-assigned this Apr 18, 2022
@jlowe
Copy link
Member Author

jlowe commented Apr 18, 2022

Keeping this as a draft until this gets some at-scale performance tests

@jlowe
Copy link
Member Author

jlowe commented Apr 18, 2022

build

@sameerz sameerz added the bug Something isn't working label Apr 18, 2022
@jlowe
Copy link
Member Author

jlowe commented Apr 19, 2022

Premerge failed due to transitive Cloudera dependency, rekicking the build.

@jlowe
Copy link
Member Author

jlowe commented Apr 19, 2022

build

Copy link
Collaborator

@abellina abellina left a comment

Choose a reason for hiding this comment

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

Minor nits

abellina
abellina previously approved these changes Apr 19, 2022
@jlowe
Copy link
Member Author

jlowe commented Apr 19, 2022

build

@jlowe
Copy link
Member Author

jlowe commented Apr 20, 2022

The latest code changes do not appear to have a significant performance impact. Thanks to @mattahrens for running some performance tests. This should now be ready for review.

@jlowe jlowe marked this pull request as ready for review April 20, 2022 18:05
@jlowe
Copy link
Member Author

jlowe commented Apr 20, 2022

build

@jlowe jlowe requested a review from abellina April 25, 2022 15:58
@jlowe
Copy link
Member Author

jlowe commented Apr 25, 2022

@revans2 @tgravescs if one of you could take a look that would be great, as it needs a codeowner approval as well.

Copy link
Collaborator

@abellina abellina left a comment

Choose a reason for hiding this comment

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

looking good, just minor comments.

revans2
revans2 previously approved these changes Apr 25, 2022
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Just a minor nit on top of what @abellina found

@jlowe
Copy link
Member Author

jlowe commented Apr 25, 2022

build

@jlowe jlowe merged commit 785b4ac into NVIDIA:branch-22.06 Apr 25, 2022
@jlowe jlowe deleted the fix-iceberg-fallback branch April 25, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Reading from iceberg table will fail.
4 participants