Repository navigation
Conversation
A Sequence with a single element has no adjacent pair to compare, so it is sorted and strictly sorted whatever its multiplier. The constructor stats, the Sequence is_sorted aggregate kernel and the generic sortedness helper all answered from the multiplier (or a cached hint) alone, so a one-row Sequence with a negative multiplier reported unsorted, and one with a zero multiplier reported not strictly sorted. Decoding also restores the stored sortedness hints, which older writers computed the same way, so a decoded one-row array now takes the intrinsic answer over those hints. Closes vortex-data#10579 Signed-off-by: Vincent <1729065730@qq.com>
Merging this PR will regress 3 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ |
Simulation | isolated_runs[u32, (16, 0)] |
78.3 µs | 98.5 µs | -20.45% |
| ❌ | WallTime | threshold_neon[80] |
3.4 µs | 3.8 µs | -10.5% |
| ❌ | WallTime | starts_with_avx2[64, VarBin] |
53.7 µs | 60 µs | -10.43% |
| ⚡ | WallTime | bitpack_blocked_compress_avx512 |
5.6 µs | 4.8 µs | +17.52% |
| ⚡ ℹ️ | Simulation | test_scan_filter_to_arrow_array[int_float] |
4.1 ms | 3.5 ms | +15.59% |
| ⚡ ℹ️ | Simulation | test_repeated_scan_filter[int_float] |
3.9 ms | 3.5 ms | +12.09% |
| ⚡ ℹ️ | Simulation | test_repeated_scan_filter[int] |
2.8 ms | 2.5 ms | +11.62% |
| ⚡ ℹ️ | Simulation | test_scan_filter_to_arrow_array[int] |
2.8 ms | 2.5 ms | +10.99% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ump45nose:fix/singleton-sequence-sortedness (de3e710) with develop (07fe4c5)
Footnotes
-
12 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. ↩
| // A single element is strictly sorted whatever the multiplier. | ||
| let result = if array.len() <= 1 { | ||
| true | ||
| } else { | ||
| let m = array.multiplier(); | ||
| match_each_native_ptype!(m.ptype(), |P| { | ||
| m.cast::<P>().map(|x| { | ||
| if options.strict { | ||
| x > zero::<P>() | ||
| } else { | ||
| x >= zero::<P>() | ||
| } | ||
| }) | ||
| })? | ||
| }; |
There was a problem hiding this comment.
The two checks cover different paths. is_sorted returns early for len <= 1, but the accumulator hands this kernel each chunk of a ChunkedArray directly, and IsSorted has no legacy stat bridge, so neither that early return nor the constructor stats are consulted there. Without the kernel check, a one-row Sequence chunk with a negative multiplier makes an otherwise sorted chunked array report unsorted. In de3e710 I added a comment in the kernel and singleton_chunk_keeps_chunked_array_sorted, which fails without the check. (AI-assisted reply.)
The Sequence is_sorted kernel keeps its own len <= 1 check even though is_sorted already returns early for such arrays: the accumulator hands the kernel each chunk of a ChunkedArray directly, so the outer check never runs for a one-row chunk. Explain this in the kernel and cover it with a chunked-array test that fails without the check. Signed-off-by: Vincent <1729065730@qq.com>
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
You should be able to consolidate the logic into one function like:
fn is_sorted(multiplier: PValue, length: usize, strict: bool) -> boolMove the length and multiplier logic into Sequence::is_sorted(multiplier, length, strict) and use it for the constructor stats and the is_sorted kernel. Signed-off-by: Vincent <1729065730@qq.com>
|
Done in d58de2d. |
This branch has not been deployed
3 participants
Summary
Closes #10579.
A one-row
Sequencehas no adjacent pair to compare, so it is sorted and strictly sorted whatever its multiplier. Its constructor stats, itsis_sortedaggregate kernel and the generic sortedness helper answered from the multiplier (or a cached hint) alone, so a negative multiplier reported unsorted and a zero multiplier reported not strictly sorted.Changes
Sequence::statsnow takes the length and reports both flags as exacttruewhen it is at most 1. This coverstry_new,try_new_typedand slicing, which goes throughnew_unchecked.SequenceIsSortedKernelreturnstruefor a one-row batch, so direct aggregate execution agrees with the stats.is_sorted_implcheckslen() <= 1before the cached statistics, so a stale hint can't contradict it.SerializedArray::decodesets both flags to exacttruefor arrays with at most one element after it restores the stored statistics. Files written before this change can store the multiplier-only answer for one-row sequences, and this makes the intrinsic fact win over those historical hints.New tests cover the typed and untyped constructors and a one-row slice of a longer sequence (multipliers -1, 0 and 3), the kernel called directly for strict and non-strict, decoding a one-row sequence whose serialized hints say unsorted, and the generic helper with stale cached stats. Apart from the positive-multiplier cases, they all fail on
develop.AI assistance
This PR was written by an AI coding agent working under my account (code, tests and this description), and I'm the responsible human for it. Points worth a reviewer's attention: the decode change in
vortex-array/src/serde.rsapplies to every encoding, not onlySequence, on the basis that any array with at most one element is (strictly) sorted; and it overwrites the stored hint with exacttruerather than dropping it.