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
DDL: Use Column Name Instead of Offset to Find the common handle cluster index #5166
DDL: Use Column Name Instead of Offset to Find the common handle cluster index #5166
Changes from 27 commits
b871522
cd92fc3
6eea5ee
8561b9a
110f2fb
d4c55a7
e68d45d
4ffb496
0128545
6e2bbb1
08b122a
8d74059
3139593
ffbd63e
fcf09ab
51e894c
6decafe
ecb26ff
d89b308
44b6798
2162411
590d4dd
238bb7b
6dfd5dd
4275d2a
d864d99
72c75bc
3ec80b2
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.
This may slow down the decoding process. I suggest not adding it or we need a test to compare the decoding time with and without this check.
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.
checkNumberOfRows
just iterate all columns and check the size of each column, I think the overhead is small.But yes, a benchmark result is better.
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 think we can create a microbenchmark to show the performance impact. i.e. a function running many rounds of
readImpl
.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 add a microbenchmark in bench_region_block_reader.cpp, which test the decode time with/without check under PKIsHandle/CommonHandle/PKIsNotHandle. I simulate the case with 100 rows and 6 columns, and run each case 1000 iters. The benchmark result shows the checkNumberOfRows only take very low percentage. Thus, I think we can add the checkNumberOfRows here.
Please look at the last four lines of these two figures, as the test in the first position will always take up additional overhead, thus we change the position to make these two tests.
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.
performance with decoding different rows