fix(datafusion): coerce numeric literals to and from Float16 - #8847
fix(datafusion): coerce numeric literals to and from Float16#8847LuciferYang wants to merge 6 commits into
Conversation
safe_coerce_scalar had no Float16 arm in either direction, so a Float16 column could not be filtered at all, and the index layer silently dropped the predicate to a refine filter.
Drive the index test through the production coercion order, pin the f16 boundary in bits, use the target-independent conversion, and stop the doc from claiming rounding is harmless.
The previous wording said the truncation was unreachable from a written literal. It is reachable: = 2049.001 matches rows holding 2048.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The new Float16 filter path must preserve two correctness invariants before it can safely feed either scans or scalar indexes: target-independent, correctly rounded literal conversion and the repository's equality/order semantics for signed zero. A viable revision should use a full-mantissa deterministic f64-to-f16 conversion and land or rebase onto a verified signed-zero fix before enabling Float16 predicates.
| /// a value differing from a tie only in those bits is treated as the tie and | ||
| /// rounds to even: `= 2049.001` matches rows holding 2048, not the nearer 2050. | ||
| fn coerce_to_f16(value: f64) -> Option<f16> { | ||
| let coerced = f16::from_f64_const(value); |
There was a problem hiding this comment.
from_f64_const makes this conversion target-stable by discarding the low 32 bits of the f64 mantissa before it rounds, but that silently chooses the wrong Float16 value for ordinary literals just above a midpoint. On this head, 2049.001 becomes 2048 even though 2050 is nearer, so both the physical filter and the scalar-index boundary can return the 2048 row for value = 2049.001. The upstream crate tracks this exact software-conversion defect in half-rs#151.
Please use a target-independent conversion that rounds from the full f64 mantissa (or reject inputs whose discarded bits can change the result). Merely switching to from_f64 is insufficient because its x86 f64 → f32 → f16 path has the separate double-rounding problem tracked in half-rs#116.
Reproducer run against 0dffbd9
A standalone program built Float16Array[-0, +0, 2048, 2050], then ran Planner::parse_filter, optimize_expr, create_physical_expr, and evaluate for value = 2049.001:
CARGO_NET_OFFLINE=true RUSTFLAGS='-C target-cpu=haswell -C target-feature=+avx2,+fma,+f16c' cargo run --offlinevalue = 2049.001: optimized=value = Float16(2048); selected=[2]
Row 2 held 2048; row 3 held the nearer value 2050.
There was a problem hiding this comment.
Fixed in 6e0ed01: 2049.001 now coerces to 2050, and the full finite-range midpoint sweep plus a two-sign boundary oracle found no rounding mismatch.
|
|
||
| #[tokio::test] | ||
| #[rstest::rstest] | ||
| #[case::float16(DataType::Float16)] |
There was a problem hiding this comment.
Adding Float16 to the filtering path makes the signed-zero comparison defect newly reachable: this head returns the -0.0 row for value < 0.0, and value = 0.0 returns only +0.0. That is a silent wrong-row result, not just missing coverage. #5868 records the required equality semantics, while its proposed fix #6236 is still open and is not in this PR's live base.
Please either rebase after a verified signed-zero fix lands or include equivalent Float16 comparison/index-boundary handling here, then extend the special-values test to Float16 with indexed and unindexed execution.
Reproducer run against 0dffbd9
Using the same Float16Array[-0, +0, 2048, 2050] planner/physical-expression program:
value < 0.0: optimized=value < Float16(0); selected=[0]
value = 0.0: optimized=value = Float16(0); selected=[1]
Row 0 held -0.0; row 1 held +0.0.
Four review rounds found a wrong clause in this one comment, each time in the sentence explaining the mechanism rather than the one stating it. The prose now stops at what it can defend and points at the cases.
from_f64 is correctly rounded on aarch64 with fp16, so the claim is only true of the const form.
Neither half entry point is correctly rounded: the software path truncates the low 32 mantissa bits before rounding (half-rs#151) and the x86 path goes through f32 (half-rs#116). Pick the true nearest among half's answer and its two neighbours, and take the overflow decision off half entirely.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
One finding is fixed; one remains. The full-mantissa rounding finding is fixed in 6e0ed01: 2049.001 now coerces to 2050, and the new conversion holds across the finite Float16 rounding boundaries.
The signed-zero finding remains. This PR makes Float16 predicates reachable, but value < 0.0 still treats -0.0 as negative and equality still misses that encoding. #6236 remains an independently mergeable prerequisite, so this PR should land after it and rebase onto its signed-zero handling, or include equivalent handling and indexed/unindexed Float16 coverage.
Problem
Fixes #8846.
A
Float16scalar column can be written and scanned, but no numeric literal ever reaches it, so it cannot be filtered at all:safe_coerce_scalarhad noFloat16arm in either direction. Both directions matter, and the second is easy to miss because it produces no error. Adding theDataType::Float16targets alone makes filters work, but the scalar index is then still never used:maybe_scalarcallssafe_coerce_scalaron a literal the planner has already coerced toFloat16, so without aScalarValue::Float16source arm the predicate silently drops to a refine filter. Rows come back correct, so only the plan shows it. Measured on a Float16 column with a BTree index, at 6 rows and at 5000:A
Float32column of the same shape and query already planned the second form, which is the control that says this is a Float16 gap and not a row-count heuristic.What this changes
DataType::Float16is now a target in all ten numeric source arms, andScalarValue::Float16is a new source arm reaching Float16, Float32 and Float64.Out-of-range literals are rejected rather than saturated. That is the one judgement call here, and it differs from the
Float64toFloat32arm next to it, so it is worth stating plainly.f16overflows at 65520, which an ordinary literal passes. Saturating100000to an infinity answersvalue < 100000correctly and makesvalue = 100000match rows that really hold infinity; collapsing1e-30to zero makesvalue = 1e-30match real zeros.safe_coerce_scalarcannot see the operator, so it cannot allow saturation only where it is harmless. The cost is thatvalue < 100000now errors where saturating would have returned every finite row. I took the error over the silently wrong row set, and left theFloat32arm alone because reaching its boundary takes a literal above 1e38.Rounding inside the range is correct, and getting there took work that is worth pointing at. Neither of
half's conversions rounds correctly: the software path truncates the low 32 bits of thef64mantissa before it rounds, so a value just above a tie goes the wrong way (half-rs#151), and the x86 hardware path rounds throughf32first, which is a different wrong answer (half-rs#116).nearest_finite_f16takes the software answer as a starting point, which at least does not vary by target, and then picks the true nearest among it and its two bit-neighbours, tie to even. Wideningf16tof64is exact, so those three comparisons decide exactly and no bit-level rounding logic is needed here. Concretely,= 2049.001now coerces to 2050 wherehalfgives 2048.Overflow does not depend on
halfat all.value.abs() >= 65520.0is the exact IEEE tie between the largest finitef16and the value that would follow it, and 65520 is exactly representable inf64, so the comparison is its own authority and the target-dependent boundary never enters into it.What remains inexact is the
f16grid itself: past 2048 it is coarser than the integers, so= 2049matches rows holding 2048. That is round-to-nearest doing its job on a 10-bit mantissa, the same way theFloat32andFloat64arms are inexact on finer grids.test_f16_rounds_to_nearest_even_across_the_whole_rangeis the check I would want a reviewer to look at first. For each of the 31743 adjacent finitef16pairs it asserts the exact midpoint and the twof64values either side of it, with the expected result stated rather than recomputed by the logic under test, so it covers the complete set of rounding decisions the function can make. Stubbing the correction back out to plainfrom_f64_constmakes it fail.Out of scope
IndexType::BloomFilterrejectsFloat16outright, so the Float16 case oftest_query_floatruns BTree, Bitmap and ZoneMap only.test_bloom_filter_rejects_float16pins that refusal so the skip cannot outlive it: when bloom filters accept Float16, that test fails next to the skip.Float16columns still have the signed-zero problem from #5868, and this PR makes it reachable, which is a fair objection to landing this first: before it, a zero comparison against a Float16 column errored, and after it the comparison runs and returns the wrong rows.value < 0.0returns the-0.0row andvalue = 0.0misses it.So this should land after #6236, which fixes #5868 and already carries the Float16 branch of the rewrite. That branch is unreachable dead code until this PR, and this PR returns wrong rows for zero predicates until that one, so the two are each other's completion and the order is forced. #6236 does not depend on this PR: it is independent of this branch, its Float16 handling is exercised by unit tests that build the scalar directly, and its suite is green without any of this. Once it lands I will rebase, drop the
halfdependency hunk and the three lockfile lines it makes redundant, and extendtest_query_float_special_valuesto Float16 over both indexed and unindexed execution, which is the coverage that section currently skips with a comment naming this gap.Test plan
cargo test -p lance-datafusion(100 passed), including 24 new cases oversafe_coerce_scalar: every numeric literal type reaching Float16, the range and tie edges pinned as raw bits, non-finite passthrough, and both zeros keeping their signcargo test -p lance-index(1176 passed), includingtest_float16_column_reaches_its_index, which drivesPlanner::parse_filterandcreate_filter_planso it runs the production coercion order. Deleting theScalarValue::Float16source arm turns all four of its cases red; thecheck_with_schemahelper would not have caught that, because it builds expressions without type coercion and so exercises the target arm instead.cargo test -p lance --lib(3175 passed)cargo test -p lance --test integration_tests --features slow_tests -- --test-threads=1(52 passed). The--features slow_testsflag is required:mod queryis behind that gate, so without it the new tests match nothing and cargo still exits 0.cargo clippy --all --tests --benches -- -D warningsclean,cargo fmt --allappliedhalfdependency onlance-datafusion