-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Support FlatHashStrategy with 100_000+ columns #21071
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
Conversation
sopel39
left a comment
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.
approach looks ok. I don't think it can impact perf, can it?
core/trino-main/src/main/java/io/trino/operator/FlatHashStrategyCompiler.java
Outdated
Show resolved
Hide resolved
|
test failures seem relevant |
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 probably should keep track of chunk first channel index, e.g: 0, 500, 1000, so that correct offsets are used when accessing: byte[] fixedChunk, byte[] variableChunk and BlockBuilder[] blockBuilders);
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 assume there is a bug like this somewhere in here, but my memory is that all of the indexes come from KeyField and not the oridinal of the key field object in the list… If you look at the code, I don’t think we are using an indexed for loop anywhere.
|
46c1603 to
d6c86b5
Compare
4f9ea39 to
48c9d16
Compare
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: