Make column order deterministic in segment #10468
Conversation
Jackie-Jiang
added
release-notes
Jackie-Jiang
force-pushed
the
metadata_deterministic_order
branch
from
7242a4c to
2c6042b
Compare
Jackie-Jiang
force-pushed
the
metadata_deterministic_order
branch
3 times, most recently
from
dddcf7d to
ce87ddb
Compare
There was a problem hiding this comment.
Does this test different column orders give same CRC?
There was a problem hiding this comment.
No, different column order will give different CRC. This test is used to make sure the order is always the same
Jackie-Jiang
force-pushed
the
metadata_deterministic_order
branch
from
ce87ddb to
3aa78db
Compare
Jackie-Jiang
force-pushed
the
metadata_deterministic_order
branch
2 times, most recently
from
056ece1 to
d9b574c
Compare
Codecov Report
@@ Coverage Diff @@
## master #10468 +/- ##
============================================
+ Coverage 64.10% 68.80% +4.70%
- Complexity 6091 6116 +25
============================================
Files 2036 2090 +54
Lines 109727 112169 +2442
Branches 16782 17080 +298
============================================
+ Hits 70338 77180 +6842
+ Misses 34236 29505 -4731
- Partials 5153 5484 +331
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 401 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Jackie-Jiang
force-pushed
the
metadata_deterministic_order
branch
from
3a29923 to
829cea0
Compare
Jackie-Jiang
force-pushed
the
metadata_deterministic_order
branch
from
829cea0 to
13457cc
Compare
| String getEndOffset(); | ||
|
|
||
| default Set<String> getAllColumns() { | ||
| default NavigableSet<String> getAllColumns() { |
There was a problem hiding this comment.
Shall we directly use TreeSet instead of NavigableSet if there is no other implementation expected .
There was a problem hiding this comment.
It is an ordered set, but not a TreeSet because the key set of the TreeMap is not a TreeSet
| .isEqual(_primaryKeyColumns, that._primaryKeyColumns) && EqualityUtils | ||
| .isEqual(_hasJSONColumn, that._hasJSONColumn); | ||
| //@formatter:off | ||
| return EqualityUtils.isEqual(_schemaName, that._schemaName) |
There was a problem hiding this comment.
why removed _fieldSpecMap and _hasJSONColumn?
There was a problem hiding this comment.
They are derived fields (similarly we don't include _dimensionNames etc.). Ideally we should define them as transient, but java ser/de still need to serialize these fields.
|
A high level question, will this handle the scenario that during schema evolution, a new column is created with default value and the CRC is still not change? |
Piggy back another high level question: why adding a column needs to change the original segment :) |
|
Thank you for fixing this! Glad to know this PR got merged! |
5 participants
In segment metadata and index map, store columns in alphabetical order so that the result is deterministic.
This is very important to ensure segment generated from the same data has the same CRC. When multiple servers consume from the same stream, it is critical that all segments generated has the same CRC, or server will need to download a new segment copy when restart due to CRC mismatch. This can cause significant longer server restart time.
Release Notes
Segment generated before/after this PR will have different CRC, so during the upgrade, we might get segments with different CRC from old and new consuming server. For the segment consumed during the upgrade, some downloads might be needed.