Update sortedness in metadata if disagree with ColumnStatistics#18998
Conversation
…e sortedness in segment metadata
Jackie-Jiang
left a comment
There was a problem hiding this comment.
When a column is sorted, we should rewrite the column metadata so that it can correctly reflect the stats. After ForwardIndexHandler, the segment should be the same as directly creating the dictionary
| handler.postUpdateIndicesCleanup(writer); | ||
| } | ||
|
|
||
| // The column now has a dictionary; isSorted must stay false and the unsorted dict forward index must read back |
There was a problem hiding this comment.
If column is sorted, we should update the column metadata and make isSorted true. Essentially it should be the same as directly generating a dictionary encoded column
There was a problem hiding this comment.
Got it. So now
- the file extension is determined based on what
statsCollector.isSorted()report, instead of from metadata - update the
isSortedin metadata tostatsCollector.isSorted()
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18998 +/- ##
============================================
+ Coverage 64.82% 65.28% +0.46%
- Complexity 1347 1405 +58
============================================
Files 3392 3418 +26
Lines 211647 214652 +3005
Branches 33302 33922 +620
============================================
+ Hits 137199 140136 +2937
+ Misses 63388 63209 -179
- Partials 11060 11307 +247
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
Before #17965, we might see a raw column is unsorted according to
metadata.propertiesbut is actually sorted (false negative). For example:So when the table config enables dictionary on this column, during the segment load/reload, the forward index rebuild in
updateIndicies. Yet the forward index creator is constructed using anAbstractColumnStatisticsCollector, which actually read through the column and report sorted.Thus a
SingleValueSortedForwardIndexCreatorwould be created here, write index to a file with a different file extension.sv.sorted.fwd. Later segment writer would try to read from.sv.unsorted.fwdbecause it determines the extension by metadata's sortedness.So we see
Change
We should follow the metadata as the source of truth. IMO the easiest change here is to override the sortedness in
IndexCreationContextwith the value in segment metadata.Test
Add an unit test to reproduce the failure scenario and verified it fails without the change, and pass with the change.