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.
PDEP-5: NoRowIndex #49694
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
PDEP-5: NoRowIndex #49694
Changes from all commits
c73892d
573bf75
2284d2a
a2dfde5
de7dbce
69680e7
8155793
997d601
75056a2
443da87
ca01f61
dfbd494
7463b1e
e2d6c58
1434726
ef0a5b2
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.
For readability, would be useful to have spaces around the
>
operator here and a few lines belowThere 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 is inconsistent with what you wrote above about having the
mode.no_row_index
optionThere 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.
how so? even when I mention it above, I make it clear that this PDEP only deals with the
NoRowIndex
class, and that this mode would be out-of-scope for this PDEPThere 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.
There is text in the PDEP that refers to the mode that makes it seem like it is part of the proposal.
For example, lines 297-298 read:
When I read that, it seems like the mode is part of the proposal. So I think you may need to qualify all references to
mode.no_row_index
to describe what would happen if themode.no_row_index
option is not available.Or make a decision to include
mode.no_row_index
as part of the proposal. (Which I think makes it easier to understand, IMHO)