Repository navigation
Conversation
Merging this PR will degrade performance by 3.73%2 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
| // 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.)
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) -> bool
|
Done in d58de2d. |
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>
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>
Move 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>
d58de2d to
766a174
Compare
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.