Fix: hardcoded DType in dict layout
#5761
Merged
+183
−29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as off-topic.
This comment was marked as off-topic.
Contributor
|
We decided that more than 64k unique elements in a DictLayout (where chunks can be ~8k elements), doesn't make much sense! I imagine this is done way with bad assumptions in a few places.. |
Contributor
|
So this was done before the dict encoder was generic over the codes ptype, and was dynamically setting the codes dtype based on the magnitude of the max_len constraint. So it was always encoding to a wider int, then after the encoding is done we would downcast to a narrower int. In that world it is not possible to tell the codes ptype before encoding is fully finished. also I don't think the codes ptype is selected dynamically, is should still be depending on the max_len argument, and if that is smaller than 256, only then we should get u8 codes: I think what is happening here is that we are trying to dict encode u8 values, in that case this code would select u8, even though max_len is u16::max. So probably the right fix is to expose the codes ptype from the dict builder? |
connortsui20
force-pushed
the
ct/fix-dict-writer
branch
2 times, most recently
from
833c131 to
6aa5e45
Compare
Contributor
Author
|
@onursatici I think that is what I am doing since I am extracting the dtype of the codes? Or are you saying that we need to forward the original dtype from somewhere else to the writer? Or are you saying we need to upcast the codes always? |
Contributor
|
I guess what I am saying is that this comment is not exactly right: the way we chose the ptype for codes is upfront, so it doesn't depend on the actual cardinality after encoding. It depends on two things, the width of the input primitive ptype, and the max_len constraint. Normally we have max_len set to u16::max because we don't want a higher cardinality in a chunk, so we always get u16 codes. In fuzzer we end up dict encoding u8 types, and because the input is narrower than the max_len constraint (u8 vs u16), dict encoder returns u8 codes. for the fix, I think this will work and I am happy to merge as is, but I think the right way is to get the codes dtype from the dict encoder, because as soon as you construct one you can get a codes ptype, you don't need to encode a chunk to see what ptype it will end up having. |
Contributor
|
Shall we do this then @connortsui20 use the max_len instead of the first batch? |
AdamGS
reviewed
Jan 8, 2026
| self.active_values_tx = Some(values_tx); | ||
|
|
||
| // Send first codes | ||
| let codes_dtype = codes.dtype().clone(); |
Contributor
There was a problem hiding this comment.
I agree with @onursatici and @joseph-isaacs, seems like we should take this from DictLayoutConstraints::max_len and not by the first batch.
Contributor
There was a problem hiding this comment.
as discussed with Joe, I will make that change and push to this branch
Deploying vortex-bench with
|
| Latest commit: |
0a7cccb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://96bde770.vortex-93b.pages.dev |
| Branch Preview URL: | https://ct-fix-dict-writer.vortex-93b.pages.dev |
Signed-off-by: Connor Tsui <[email protected]>
Signed-off-by: Onur Satici <[email protected]>
onursatici
force-pushed
the
ct/fix-dict-writer
branch
from
0a7cccb to
11a0cbb
Compare
Comment on lines
+53
to
+58
| /// Constraints for dictionary layout encoding. | ||
| /// | ||
| /// Note that [`max_len`](Self::max_len) is limited to `u16` (65,535 entries) by design. Since | ||
| /// layout chunks are typically ~8k elements, having more than 64k unique values in a dictionary | ||
| /// means dictionary encoding provides little compression benefit. If a column has very high | ||
| /// cardinality, the fallback encoding strategy should be used instead. |
Contributor
There was a problem hiding this comment.
might be worth saying you can still use a DictArray?
Contributor
joseph-isaacs
left a comment
joseph-isaacs
left a comment
There was a problem hiding this comment.
Shall we let the tests finish before we merge
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
joseph-isaacs
approved these changes
Jan 13, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #5591
Also fixes #5563
Still not super sure about why we limit the dict layout codes to be a max of
u16::MAX...