[ABA-20] fix(vortex-array): reject fractional f64 values in integer casts#10
Open
abnobdoss wants to merge 2 commits into
Open
[ABA-20] fix(vortex-array): reject fractional f64 values in integer casts#10abnobdoss wants to merge 2 commits into
abnobdoss wants to merge 2 commits into
Conversation
…s fractional values Add a failing regression test `issue_aba20_cast_f64_to_i64_rejects_fractional_values` that documents the bug: casting 1.5_f64 to i64 currently returns Ok(1) instead of Err. Also rename the test module from `test` to `tests` per project conventions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Abanoub Doss <abanoub.doss@gmail.com>
In the array kernel (CastKernel for Primitive), add a validity-aware per-element check before any float→int element-wise cast: iterate valid positions only and return a Compute error if any has a non-zero fractional part (detected via f64::fract() != 0.0). In the scalar layer (PValue::cast), add the same fractional check when the source is a float variant (F16/F32/F64) and the target ptype is an integer. For F16, promote to f32 via ToPrimitive before calling fract(). Both fixes correctly handle NaN and ±Inf (their fract() returns NaN, which compares unequal to 0.0, so they are rejected). In practice NaN and ±Inf are already caught by the `values_fit_in` range guard, but the fractional check provides a second, more precise layer. The PValue::cast fix also causes the conformance test helper `fits()` to return false for fractional min/max values, keeping the conformance suite consistent without modifying any test data. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Abanoub Doss <abanoub.doss@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
f64(orf32/f16) array with fractional values to an integer type silently truncated the fractional part instead of returning an error. For example,cast([1.5_f64] → i64)returnedOk([1])instead ofErr.CastKernel::castdelegated tocast::<F, T>()which usesAsPrimitive::as_()— the Rustasoperator — which silently truncates. The pre-flight guardvalues_fit_inchecked only min/max range viaPValue::cast, andPValue::castusedNumCast::from(1.5_f64)which returnsSome(1), so fractional values slipped through.reject_fractional_values()whensource.is_float() && target.is_int(). This materializes the validity mask and checksfract() != 0.0on valid positions only, skipping null slots.PValue::castgains the same fractional check for float→int casts. This also makes the conformance test helperfits()returnfalsefor fractional values, keeping the conformance suite self-consistent without changing test data.Linear
Closes ABA-20
Validation
issue_aba20_cast_f64_to_i64_rejects_fractional_values(inline#[cfg(test)]):cast([1.5_f64] → i64)→Err✓cast([2.0_f64] → i64)→Ok([2])✓cast([Some(2.0_f64), None, Some(3.0)] → nullable i64)→Ok([Some(2), None, Some(3)])✓ (null slots with potentially fractional underlying bits do not trigger false positives)cargo test -p vortex-array: 2753 unit tests + 67 doc tests pass, 0 failuresvalues_fit_in; the fractional check also catches them as a secondary layer (theirfract()is NaN ≠ 0.0).Note: Scalar callers of
PValue::castoutsidevortex-array(e.g. invortex-datafusion,vortex-session) may previously have relied on silent truncation semantics. This fix makes scalar float→int cast strict; downstream callers should be reviewed if they perform such casts intentionally.🤖 Generated with Claude Code