Repository navigation
duckdb: i16 Decimal on duckdb side may fit in Vortex's i8 decimal buffer - #10155
Conversation
myrrc
added
changelog/fix
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| Simulation | bench_compare_sliced_dict_primitive[(3333, 10000)] |
76.6 µs | < 1 ns | N/A |
Comparing myrrc/duckdb-decimal-null-exporter (4beb1d4) with develop (dd1ce26)
Footnotes
-
518 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
I really don't understand here. What is failing the byte-widths are the same. Can we add a test showing it or is that too hard? |
|
When we're writing the file, we're converting from a NULL
value of type DECIMAL(2, 1) (i16) from Duckdb, which contains garbage,
to a Vortex value of type i8 which is the underlying storage for our
Decimal(2, 1). Garbage fitting in i16 doesn't always fit in i8.
I don't think you can easily write a test for this because the
state of Duckdb's uninitialized memory can't be tinkered with.
You can try writing from a duckdb table with nullable decimal(2, 1) to
a Vortex file with same decimal, not sure it would reproduce the bug
though
|
|
I am also lost - we clearly copy same values. Where do we produce the i8 array from i16? Seems like that conversion logic is wrong and panics if null value is out of the narrowed range? |
|
If this is the case the fix should be in narrowing function not in duckdb |
myrrc
changed the title
|
These are two separate bugs, both in decimal code. I've updated the patch. |
myrrc
changed the title
myrrc
changed the title
Signed-off-by: Mikhail Kot <[email protected]>
8d0325e to
526a691
Compare
Signed-off-by: Mikhail Kot <[email protected]>
Signed-off-by: Mikhail Kot <[email protected]>
Signed-off-by: Mikhail Kot <[email protected]>
Decimal builders have an extend() method which adds values to the builder's decimal buffer from another decimal buffer. We allow extend()ing from a decimal buffer which is greater than our builder's buffer. This may happen with a Chunked array where for a given precision we've estimated values to fit in T but one of chunks has type Y >= T.
As a particular example, it happens in Duckdb with a Decimal(2, 1). For 2 our type is i8 but Duckdb's minimal type for Decimals is i16, so we do a narrowing i16->i8 conversion.
Inside extend(), we lazily convert the values from the extending buffer to the target type and fail on conversion. We also don't consider NULL values. So, a NULL value on an extending buffer which contents don't fit into a builder's decimal buffer will fail a conversion. This in particular happens with copy_to_stats SLT under nextext.
The correct fix for this is to ensure boundaries between a query engine and Vortex are handled correctly, so we need to to an extra pass in the input buffer and zero out values under NULL. However, this is a big change to build and invariant.
The simpler fix, though, is to handle specific i16->i8 conversion which I do here.