Enable segmentwise recompression for nullable order by columns with firstlast metadata index - #9963
Conversation
|
@Poroma-Banerjee, @antekresic: please review this pull request.
|
| /* compressed boundary is NULL */ | ||
| if (is_null) | ||
| { | ||
| if (min_bound) |
There was a problem hiding this comment.
Not sure what this means. Can there be a comment to explain?
There was a problem hiding this comment.
This logic was moved to slot_key_test as it's a better fit, see comments there.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
eafe9ea to
e8f3046
Compare
natalya-aksman
added
Columnstore
|
@kpan2034, @pnthao: please review this pull request.
|
| * | ||
| * If slot attribute is NULL and key is NOT NULL, | ||
| * (key >= NULL) returns True for nulls_first | ||
| * and (key <= NULL) returns True for !nulls_first (i.e. for NULLS LAST). |
There was a problem hiding this comment.
what is (key <= NULL)?
not-NULL key value sorts before NULL, the comment can be changed to be more accurate.
|
is issue #9970 related? |
No, that issue is about renaming a column which is used in firstlast metadata, because metadata column names for firstlast are based on source column names, for example if we had firstlast index on column I.e. #9970 is completely unrelated. |
| } | ||
|
|
||
| /* NULL > key i.e. NULL sorts after key argument */ | ||
| if (!nulls_first && (key->sk_strategy == BTGreaterStrategyNumber || |
There was a problem hiding this comment.
Now that you have this check here, is handle_null_scan still needed separately?
There was a problem hiding this comment.
Yes, because the change in slot_key_test is about matching a boundary to the scan key, this method returns True if scan key fits the boundary and False otherwise, for example it is True for boundary = 5 and scan key (6, >=), i.e. (6 >= 5) = True.
We can now have Tuple_match result when key is not NULL but the boundary is NULL, i.e. for boundary = NULL, nullfirst = True and scan key (6, >=) we used to return False (no match) and now we return True (match as 6 sorts after NULL).
But we still need to handle cases of key = NULL or boundary = NULL when slot_key_test returns False. We need to check whether this tuple should go before or after a region defined by those boundaries, for example 6 goes before [10, NULL] for NULLS LAST scenario. So no change in logic for handle_null_scan, we just now return True from slot_key_test in more scenarios.
…irstlast metadata index
e8f3046 to
75cbaac
Compare
…irstlast metadata index (timescale#9963) Removes restrictions on segmentwise recompression with nullable order by columns introduced to address timescale#9444. Now it is OK to segmentwise-recompress on nullable order by columns with firstlast indexes as [timescale#9784](timescale#9784) have been implemented. Using firstlast metadata with NULLs exposed a bug in `slot_key_test` in regards to matching a scan key to batches like [NULL, 10] or [10, NULL] which did not exist with minmax indexes, the bug is fixed now.
4 participants
Removes restrictions on segmentwise recompression with nullable order by columns introduced to address #9444.
Now it is OK to segmentwise-recompress on nullable order by columns with firstlast indexes as #9784 have been implemented.
Using firstlast metadata with NULLs exposed a bug in
slot_key_testin regards to matching a scan key to batches like [NULL, 10] or [10, NULL] which did not exist with minmax indexes, the bug is fixed now.To enable segmentwise recompression for firstlast indexes it is also advisable to address leading column limitation on matching tuples to batches on multiple orderby keys. It was necessary limitation for minmax index but multikeys can be properly matched with firstlast indexes.
Reworking multikey tuple to batch matching for firstlast could be done in a different PR.
Disable-check: force-changelog-file