Skip to content

fix(vortex-row): decimal sort keys are not memcmp-comparable across chunks#8937

Merged
HarukiMoriarty merged 7 commits into
developfrom
nemo/row-decimal-key-width
Jul 24, 2026
Merged

fix(vortex-row): decimal sort keys are not memcmp-comparable across chunks#8937
HarukiMoriarty merged 7 commits into
developfrom
nemo/row-decimal-key-width

Conversation

@HarukiMoriarty

@HarukiMoriarty HarukiMoriarty commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

The bug

ORDER BY / top-k on a decimal column silently returns wrongly ordered rows whenever the column spans chunks whose physical value widths differ. No error is raised; the output looks plausible.

Example

Take a DECIMAL(7, 5) tip column and ORDER BY tip DESC. Two rows land in different chunks, and compression picks a different physical width for each chunk:

row unscaled value chunk's physical type
tip = 0.00484 484 i16 (484 doesn't fit i8)
tip = 0.00091 91 i8 (every value in its chunk fits i8)

A descending sort key is built per chunk: write the value big-endian, flip the sign bit, invert the value bytes (for DESC), and prefix the non-null sentinel 0xFE. Before this fix, the number of value bytes came from the chunk's type:

tip = 0.00484, encoded at its chunk's width (i16 → 2 value bytes)

    big-endian bytes      01 E4
    flip sign bit         81 E4
    invert for DESC       7E 1B
    prepend sentinel   FE 7E 1B      ← 3-byte key

tip = 0.00091, encoded at its chunk's width (i8 → 1 value byte)

    big-endian bytes      5B
    flip sign bit         DB
    invert for DESC       24
    prepend sentinel   FE 24         ← 2-byte key

The sorter compares keys with memcmp:

key(0.00484) = FE 7E 1B
key(0.00091) = FE 24
               ─┬ ─┬
                │  └─ byte 1 decides: 0x24 < 0x7E, so key(0.00091) is the smaller key
                └─ byte 0: equal

But byte 1 means different things in the two keys: in the 3-byte key it is the high-order byte of a two-byte number, while in the 2-byte key it is the only byte of a one-byte number. The comparison is meaningless — and DESC encoding promises bigger value ⇒ smaller key, so the smaller key wins: the sorter ranks 0.00091 as a larger tip than 0.00484. Ascending breaks symmetrically.

With this fix, both chunks encode at the width the declared dtype implies (DECIMAL(7,5) → i32 → 4 value bytes), whatever their physical storage:

key(0.00484) = FE 7F FF FE 1B
key(0.00091) = FE 7F FF FF A4
                        ─┬
                         └─ byte 3 decides: 0xFE < 0xFF, so key(0.00484) is smaller

Equal-length keys, every byte position aligned — 0.00484 correctly sorts first in the descending order.

The fix

Derive the key width from the declared dtype, so every chunk of a column encodes identical-length keys:

  • decimal_key_type (vortex-row): the dtype-derived width, used by both the sizing pass and the encode dispatch.
  • converted_buffer<W> (vortex-array, next to widened_buffer): returns a chunk's values at exactly width W. Zero-copy when the chunk is already stored at W (the common case), lossless widening otherwise, and an error for any value that violates its declared precision, such values previously encoded a garbage key silently.

The encode loop itself is unchanged; only where it reads its values from changed.

Decimal chunks of one column compress to different physical value widths,
but the row encoder sized and encoded keys at the chunk's values_type().
Keys from differently compressed chunks therefore had different lengths,
and memcmp between them no longer matched value order, silently corrupting
any ORDER BY / top-k over a multi-chunk decimal column.

Derive the key width from the declared decimal dtype instead (the same
rule row_width_for_dtype already uses), and bring each chunk's physical
values to that width via a new converted_buffer helper in vortex-array:
zero-copy when the widths already match, lossless widening otherwise, and
an error for any value that violates its declared precision (previously
encoded silently).

Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Comment thread vortex-array/src/arrays/decimal/utils.rs Outdated

@a10y a10y left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for finding this! Let's please add a regression test in vortex-row, just the case from the PR description is a good starting point

@a10y

a10y commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

We probably should have a fuzz target too for vortex-row :/ Doesn't need to be in this PR but probably a good follow up

Additionally, had claude take a look, it notes

It also quietly fixes a second, worse latent bug the PR body doesn't mention: for decimals nested in structs/FSLs, the slot width was already computed from the declared dtype (row_width_for_dtype, codec.rs:797, codec.rs:846) while encode_decimal wrote bytes at the chunk's physical width. A nested decimal chunk stored at a non-declared width would desynchronize col_offset and corrupt every subsequent column's bytes in the row.

robert3005 and others added 4 commits July 24, 2026 12:14
Null slots hold unspecified backing bytes, so converted_buffer could
report a spurious overflow for a value the encoder never reads. Take
the validity mask and validate-then-convert: only valid values must
fit the target width (checked before the mask, so the happy path never
touches it), then an infallible vectorized conversion. Pure widening
routes through widened_buffer and needs no validation at all.

Also route row_width_for_dtype's decimal arm through decimal_key_type
so the size plan and the encoder share one width rule.

Signed-off-by: Nemo Yu <zyu379@wisc.edu>
- keys from chunks with different physical widths compare like their
  values, ascending and descending (fails on the pre-fix encoder)
- garbage backing a null slot encodes without a spurious overflow
- a valid value that does not fit the dtype-derived key width fails
  loudly instead of encoding a corrupt key

Signed-off-by: Nemo Yu <zyu379@wisc.edu>
…ey-width

Signed-off-by: Nemo Yu <zyu379@wisc.edu>

# Conflicts:
#	vortex-row/src/tests.rs
@HarukiMoriarty
HarukiMoriarty requested a review from a10y July 24, 2026 14:47
@HarukiMoriarty
HarukiMoriarty enabled auto-merge (squash) July 24, 2026 14:49
@codspeed-hq

codspeed-hq Bot commented Jul 24, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 1849 untouched benchmarks
⏩ 46 skipped benchmarks1


Comparing nemo/row-decimal-key-width (e0a2a24) with develop (d9619b8)

Open in CodSpeed

Footnotes

  1. 46 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.

Comment on lines +50 to +51
// Pass 1: only *valid* values must fit `W`; null-slot garbage is exempt. Checking
// overflow before the mask keeps mask lookups off the happy path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this true? this still forces a branch in the inner loop

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should handle when validity is alltrue or nonnullable without a branch, and then if the validity is real Mask then we do the branch

The narrowing validation scanned with a short-circuiting find, which
branches per element and consults the mask on every overflow. Detect
overflow with a non-short-circuiting fold instead, so the scan
vectorizes; the mask-aware rescan only runs once an overflow exists.

Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
@HarukiMoriarty
HarukiMoriarty requested a review from a10y July 24, 2026 16:46

@a10y a10y left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@HarukiMoriarty
HarukiMoriarty merged commit 292ff08 into develop Jul 24, 2026
74 checks passed
@HarukiMoriarty
HarukiMoriarty deleted the nemo/row-decimal-key-width branch July 24, 2026 18:06
HarukiMoriarty added a commit that referenced this pull request Jul 24, 2026
Follow-up to #8937, where @a10y suggested a fuzz target for vortex-row.

## What it does

Adds a `row_encode` fuzz target that generates 1–3 same-length columns
of arbitrary dtypes (via the existing `ArbitraryArray` machinery) with
independent per-column `RowSortField` configs (descending ×
nulls-first), and checks three properties:

1. **Sizes**: `compute_row_sizes`' per-row `{fixed, var}` totals equal
the encoded key lengths.
2. **Order**: byte comparison of any two keys equals a logical tuple
comparison of the row values, asserted pairwise. The oracle is
independent of the encoder: it lifts each row into a `RowKey` (recursive
over structs/FSLs) and compares with the byte format's exact semantics,
nulls positioned by `nulls_first` regardless of direction, and
`descending` inverting leaf values only.
3. **Cross-chunk**: keys from *separate* encode calls over different
slices of the same columns, still compare correctly against each other.
This is the contract sort/top-k operators rely on when comparing keys
across batches.

Unsupported dtypes (List, Variant, Union, Extension, Decimal256) assert
that the encoder rejects them. The target is wired into the scheduled
fuzz workflow like the existing targets.

---------

Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants