-
Notifications
You must be signed in to change notification settings - Fork 848
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
Refactor compression code to reduce duplication #5593
Conversation
e011bc9
to
8fc65cd
Compare
Codecov Report
@@ Coverage Diff @@
## main #5593 +/- ##
==========================================
- Coverage 90.54% 90.41% -0.13%
==========================================
Files 229 229
Lines 47525 54023 +6498
==========================================
+ Hits 43031 48847 +5816
- Misses 4494 5176 +682
... and 203 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -248,6 +248,8 @@ typedef struct RowCompressor | |||
int64 num_compressed_rows; | |||
/* if recompressing segmentwise, we must know this so we can reset the sequence number */ | |||
bool segmentwise_recompress; | |||
/* flag for checking if we are working on the first tuple */ | |||
bool first_iteration; |
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 bool
would not be needed in case row_compressor_new_row_is_in_new_group()
could answer true on the first call
I had some not-so-nice ideas like:
- init segment_info with
is_null
but with1
value row_compressor->rows_compressed_into_current_value
is only0
during the first invocation; after that we will always have an open row with>=1
entries
I think they are not necessarily better - I'll just leave them here for a second consideration by you...
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.
Had some similar ideas but decided to go with this explicit way for a couple of reasons:
- matches the previous code more closely
- it makes a bit more understandable without possibility to introduce subtle bugs by relying on other fields
Disable-check: force-changelog-changed