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
Materialize scan results correctly when columns are not present in the segments #16619
Materialize scan results correctly when columns are not present in the segments #16619
Changes from 4 commits
5f86bea
886439b
a95807d
e66b948
591e25c
180f9fe
9d64ed4
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.
....what if the signature changes - is that a problem? shouldn't that be an Exception?
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.
if there are two cursors, CursorA with RowSignatureA and CursorB with RowSignatureB and the cursor is at the last row of CursorA, populate call will return
false
, i.e. the two cursors cannot be batched together, and setcurrentRowSignature
to the RowSignatureB (i.e. prepare the variables for the next write). We still want to return the old frame with the old signature therefore we need to preserve the signature with which we have written the frame.Per your previous suggestion,
frameWriterFactory.signature()
would be sufficient and cleaner, and I will use that instead.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.
note: I wonder why use a fastutil
IntList
- if it gets iterated with aforeach
; plainget
?this could be moved into some method like
validateRow
- that will naturally do a CSE of thecurrentRows.get(currentRowIndex)
so that it will be only evaluated onceThere 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.
No reason to use FastUtil IntList as such. I just thought it might be faster to create than an arraylist.
It is getting evaluated once here right? Unless I misinterpreted your 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.
this was just a note; this loop is validating one row; but to access that it has to do a function call
currentRows.get(currentRowIndex)
; which became part of the loop body - moving it into a method could make it clear that it works on a row - and it will naturally remove thecurrentRows.get(currentRowIndex)
as that's the row :)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.
note: isn't this comment misplaced? (note: this detail is not necessary - but it could live as an apidoc of the
validateRow
if that would be around)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.
Cleaned up the code