Repository navigation
Guard the DecimalByteParts nullability cast rule - #10746
connortsui20 wants to merge 1 commit into
Conversation
The DecimalByteParts cast rule cast the most significant part for every target nullability, even when `Validity::trivially_cast_nullability` could not prove the cast safe. The rule now declines in that case, and a new `CastKernel` handles it at execution with `cast_nullability`. Casts that change the precision or the scale stay unsupported. The cast tests now use a session that registers the crate, so they reach the new kernel. Signed-off-by: Connor Tsui <[email protected]>
Merging this PR will degrade performance by 1.68%4 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
|
im wondering if we should just solve #10750 more directly? i dont know if this is the correct general fix |
|
I have been auditing them in the past. If you have an idea how to fix it make a pr |
|
we should also just merge this since it fixes a bug |
|
Actually I think this is not a bug, we still get the same result no matter what because the pushed down cast fails regardless. I think this is a question of implementation / who should handle this generally |
|
ok we talked offline, even though this change is technically fine, it doesnt really do that much (it is not actually a bug) and we should just make sure everything similar to this is handled in the same way. |
|
Follow-up: #10798 documents this on Generated by Claude Code |
## Summary - Closes #10750 - Replaces #10746 The docs on `Validity::cast_nullability` read as if every `CastReduce` rule that removes nullability must call `Validity::trivially_cast_nullability` and pair with a `CastKernel`. Most push-down rules (FoR, ALP, ALP-RD, ZigZag, RLE, RunEnd, DateTimeParts, DecimalByteParts, Chunked, Sparse, Extension) instead cast the child that holds their validity, and the cast of that child already fails on nulls, so no result is wrong. Guarding each rule, as #10746 does for DecimalByteParts, adds a kernel per encoding and delays the rewrite until execution without changing any result. The guard in #10746 comes from the closed #10230, where a canonical decimal held a lazy child that a cast to canonical did not execute, by way of the FX2 notes in #10633. `develop` keeps one buffer per decimal, so that reason no longer applies. ## Changes `CastReduce` now owns the contract and documents both forms of rule. A rule that builds a new `Validity` keeps the guard and the kernel, and a rule that pushes the cast into its children needs neither when the nulls of those children are exactly the null rows of the array. `Dict` is the documented exception, because its values can hold nulls that no code references. No new tests are needed: every push-down encoding listed above already runs `test_cast_conformance` on a partially valid array, and that suite asserts that the cast to non-nullable fails. --- _Generated by [Claude Code](https://claude.ai/code/session_01V1YHcosAAKufkyKPorUC2n)_
Summary
The DecimalByteParts cast rule pushed every nullability cast into its most significant part (MSP), including casts to non-nullable that it could not prove safe. The cast-rule contract on
Validity::cast_nullabilitysays that aCastReducerule, which runs in the optimizer without execution, must decline whenValidity::trivially_cast_nullabilitycannot decide, and that aCastKernelhandles the cast at execution. No wrong result is visible today, because the null check still runs when the MSP cast executes, but the rule relied on every MSP encoding keeping that check.Changes
The rule now declines when
trivially_cast_nullabilityreturnsNone. A newCastKernelchecks the validity withcast_nullabilityand then rebuilds the array over the cast MSP, reusing the lower parts. Casts that change the precision or the scale stay unsupported.When the MSP has a validity array, a cast to non-nullable now stays a lazy
Castover the DecimalByteParts array until execution, instead of becoming a DecimalByteParts array in the optimizer. The MSP's own cast kernel checks the validity a second time, which for a primitive MSP is a lookup of the cached minimum.The cast tests now use a session that registers the crate, so they reach the new kernel. Seven other encodings push nullability casts the same way, tracked in #10750.
🤖 Generated with Claude Code