From 418926fa79916de34a886f6d476817669a7685b5 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Fri, 28 Aug 2026 23:21:20 +0800 Subject: [PATCH 1/6] fix(datafusion): coerce numeric literals to and from Float16 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. --- Cargo.lock | 1 + java/lance-jni/Cargo.lock | 1 + python/Cargo.lock | 1 + rust/lance-datafusion/Cargo.toml | 1 + rust/lance-datafusion/src/expr.rs | 171 ++++++++++++++++++++++ rust/lance-index/src/scalar/expression.rs | 55 +++++++ rust/lance/tests/query/primitives.rs | 24 +-- 7 files changed, 244 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7650ea8341b..132ab91cd73 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4629,6 +4629,7 @@ dependencies = [ "datafusion-physical-expr", "datafusion-substrait", "futures", + "half", "jsonb", "lance-arrow", "lance-core", diff --git a/java/lance-jni/Cargo.lock b/java/lance-jni/Cargo.lock index 62754776f90..1cc75e4c143 100644 --- a/java/lance-jni/Cargo.lock +++ b/java/lance-jni/Cargo.lock @@ -3859,6 +3859,7 @@ dependencies = [ "datafusion-physical-expr", "datafusion-substrait", "futures", + "half", "jsonb", "lance-arrow", "lance-core", diff --git a/python/Cargo.lock b/python/Cargo.lock index d0ae6f07b11..c8739047bc2 100644 --- a/python/Cargo.lock +++ b/python/Cargo.lock @@ -4184,6 +4184,7 @@ dependencies = [ "datafusion-physical-expr", "datafusion-substrait", "futures", + "half", "jsonb", "lance-arrow", "lance-core", diff --git a/rust/lance-datafusion/Cargo.toml b/rust/lance-datafusion/Cargo.toml index 7dfa455f0d0..8b9c702d2fb 100644 --- a/rust/lance-datafusion/Cargo.toml +++ b/rust/lance-datafusion/Cargo.toml @@ -24,6 +24,7 @@ datafusion-physical-expr.workspace = true datafusion-substrait = {workspace = true, optional = true} datafusion.workspace = true futures.workspace = true +half.workspace = true jsonb = {workspace = true} lance-arrow.workspace = true lance-core = {workspace = true, features = ["datafusion"]} diff --git a/rust/lance-datafusion/src/expr.rs b/rust/lance-datafusion/src/expr.rs index 6618a4f7cab..547cef6a623 100644 --- a/rust/lance-datafusion/src/expr.rs +++ b/rust/lance-datafusion/src/expr.rs @@ -9,9 +9,35 @@ use arrow::compute::cast; use arrow_array::{ArrayRef, cast::AsArray}; use arrow_schema::{DataType, TimeUnit}; use datafusion_common::ScalarValue; +use half::f16; const MS_PER_DAY: i64 = 86400000; +/// Coerce a float to `f16`, rejecting a finite value that does not survive the +/// much narrower `f16` range. +/// +/// Rounding within the range is fine and matches the integer-to-float arms +/// below: a literal that lands on a nearby `f16` still filters sensibly. +/// Saturating does not, because this function cannot see the operator it is +/// coercing for. `value < 100000` would answer correctly once the literal +/// became infinity, but `value = 100000` would then match rows that really hold +/// infinity, and `value = 1e-30` collapsed to zero would match real zeros. An +/// infinite or NaN input converts faithfully and is kept. +/// +/// `Float64` to `Float32` saturates rather than rejecting. Reaching that takes a +/// literal above 1e38, while `f16` overflows at 65520, which ordinary data +/// passes. +fn coerce_to_f16(value: f64) -> Option { + let coerced = f16::from_f64(value); + if coerced.is_infinite() && !value.is_infinite() { + return None; + } + if coerced == f16::ZERO && value != 0.0 { + return None; + } + Some(coerced) +} + // This is slightly tedious but when we convert expressions from SQL strings to logical // datafusion expressions there is no type coercion that happens. In other words "x = 7" // will always yield "x = 7_u64" regardless of the type of the column "x". As a result, we @@ -47,6 +73,9 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option { val.and_then(|v| u64::try_from(v).map(|v| ScalarValue::UInt64(Some(v))).ok()) } + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(f32::from(v)))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(f64::from(v)))), _ => None, @@ -70,6 +99,9 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option { val.and_then(|v| u64::try_from(v).map(|v| ScalarValue::UInt64(Some(v))).ok()) } + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(f32::from(v)))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(f64::from(v)))), _ => None, @@ -98,6 +130,9 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(v as f32))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(v as f64))), _ => None, @@ -126,6 +161,9 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(v as f32))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(v as f64))), DataType::Decimal128(_, _) | DataType::Decimal256(_, _) => value.cast_to(ty).ok(), @@ -142,6 +180,9 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option val.map(|v| ScalarValue::UInt16(Some(u16::from(v)))), DataType::UInt32 => val.map(|v| ScalarValue::UInt32(Some(u32::from(v)))), DataType::UInt64 => val.map(|v| ScalarValue::UInt64(Some(u64::from(v)))), + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(f32::from(v)))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(f64::from(v)))), _ => None, @@ -161,6 +202,9 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option Some(value.clone()), DataType::UInt32 => val.map(|v| ScalarValue::UInt32(Some(u32::from(v)))), DataType::UInt64 => val.map(|v| ScalarValue::UInt64(Some(u64::from(v)))), + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(f32::from(v)))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(f64::from(v)))), _ => None, @@ -185,6 +229,9 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option Some(value.clone()), DataType::UInt64 => val.map(|v| ScalarValue::UInt64(Some(u64::from(v)))), // See above warning about lossy float conversion + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(v as f32))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(v as f64))), _ => None, @@ -213,16 +260,31 @@ pub fn safe_coerce_scalar(value: &ScalarValue, ty: &DataType) -> Option Some(value.clone()), // See above warning about lossy float conversion + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(v as f64).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(v as f32))), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(v as f64))), _ => None, }, + ScalarValue::Float16(val) => match ty { + DataType::Float16 => Some(value.clone()), + DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(v.to_f32()))), + DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(v.to_f64()))), + _ => None, + }, ScalarValue::Float32(val) => match ty { + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(f64::from(v)).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => Some(value.clone()), DataType::Float64 => val.map(|v| ScalarValue::Float64(Some(f64::from(v)))), _ => None, }, ScalarValue::Float64(val) => match ty { + DataType::Float16 => { + val.and_then(|v| coerce_to_f16(v).map(|v| ScalarValue::Float16(Some(v)))) + } DataType::Float32 => val.map(|v| ScalarValue::Float32(Some(v as f32))), DataType::Float64 => Some(value.clone()), _ => None, @@ -826,6 +888,115 @@ mod tests { ); } + /// Every numeric literal type reaches `Float16`. SQL only produces `Int64` + /// and `Float64`, but `safe_coerce_scalar` is public and the index layer + /// feeds it whatever the planner already coerced, including `Float16` + /// itself. + #[rstest::rstest] + #[case::int8(ScalarValue::Int8(Some(-2)))] + #[case::int16(ScalarValue::Int16(Some(-2)))] + #[case::int32(ScalarValue::Int32(Some(-2)))] + #[case::int64(ScalarValue::Int64(Some(-2)))] + #[case::float32(ScalarValue::Float32(Some(-2.0)))] + #[case::float64(ScalarValue::Float64(Some(-2.0)))] + #[case::float16(ScalarValue::Float16(Some(f16::from_f32(-2.0))))] + fn numeric_literals_coerce_to_f16(#[case] value: ScalarValue) { + assert_eq!( + safe_coerce_scalar(&value, &DataType::Float16), + Some(ScalarValue::Float16(Some(f16::from_f32(-2.0)))), + ); + } + + #[rstest::rstest] + #[case::uint8(ScalarValue::UInt8(Some(2)))] + #[case::uint16(ScalarValue::UInt16(Some(2)))] + #[case::uint32(ScalarValue::UInt32(Some(2)))] + #[case::uint64(ScalarValue::UInt64(Some(2)))] + fn unsigned_literals_coerce_to_f16(#[case] value: ScalarValue) { + assert_eq!( + safe_coerce_scalar(&value, &DataType::Float16), + Some(ScalarValue::Float16(Some(f16::from_f32(2.0)))), + ); + } + + /// A `Float16` literal also has to reach the wider float columns, so a + /// predicate written against one column type still filters another. + #[test] + fn test_f16_literal_widens() { + let half = ScalarValue::Float16(Some(f16::from_f32(0.5))); + assert_eq!( + safe_coerce_scalar(&half, &DataType::Float32), + Some(ScalarValue::Float32(Some(0.5))), + ); + assert_eq!( + safe_coerce_scalar(&half, &DataType::Float64), + Some(ScalarValue::Float64(Some(0.5))), + ); + assert_eq!(safe_coerce_scalar(&half, &DataType::Int32), None); + } + + /// Rounding inside the `f16` range is accepted; leaving the range is not. + /// Saturating to infinity or collapsing to zero would answer an ordered + /// comparison correctly and an equality wrongly, and this function cannot + /// see which operator it is coercing for. + #[rstest::rstest] + // Rounds to the nearest representable f16 (0.1 is not exact in any binary float). + #[case::rounds(0.1, Some(0.1_f32))] + #[case::largest_finite(65504.0, Some(65504.0))] + #[case::smallest_subnormal(6e-8, Some(6e-8))] + // Overflows the f16 range rather than saturating to infinity. + #[case::overflow(70000.0, None)] + #[case::negative_overflow(-70000.0, None)] + #[case::f32_max(f32::MAX as f64, None)] + // Underflows to zero rather than silently matching real zeros. + #[case::underflow(1e-30, None)] + #[case::negative_underflow(-1e-30, None)] + fn test_f16_range_edges(#[case] input: f64, #[case] expected: Option) { + let coerced = safe_coerce_scalar(&ScalarValue::Float64(Some(input)), &DataType::Float16); + match expected { + Some(expected) => assert_eq!( + coerced, + Some(ScalarValue::Float16(Some(f16::from_f32(expected)))), + ), + None => assert_eq!(coerced, None), + } + } + + /// A literal that is already infinite or NaN converts faithfully. Only a + /// finite value that overflowed is rejected. + #[test] + fn test_f16_keeps_non_finite_literals() { + for (input, expected) in [ + (f64::INFINITY, f16::INFINITY), + (f64::NEG_INFINITY, f16::NEG_INFINITY), + ] { + assert_eq!( + safe_coerce_scalar(&ScalarValue::Float64(Some(input)), &DataType::Float16), + Some(ScalarValue::Float16(Some(expected))), + ); + } + let nan = safe_coerce_scalar(&ScalarValue::Float64(Some(f64::NAN)), &DataType::Float16); + assert!(matches!(nan, Some(ScalarValue::Float16(Some(v))) if v.is_nan())); + } + + /// Both zeros survive as themselves. Signed-zero comparison semantics are a + /// separate problem (#5868); coercion must at least not erase the sign. + #[test] + fn test_f16_keeps_zero_sign() { + assert_eq!( + safe_coerce_scalar(&ScalarValue::Float64(Some(-0.0)), &DataType::Float16), + Some(ScalarValue::Float16(Some(f16::NEG_ZERO))), + ); + assert_eq!( + safe_coerce_scalar(&ScalarValue::Float64(Some(0.0)), &DataType::Float16), + Some(ScalarValue::Float16(Some(f16::ZERO))), + ); + assert_eq!( + safe_coerce_scalar(&ScalarValue::Int64(Some(0)), &DataType::Float16), + Some(ScalarValue::Float16(Some(f16::ZERO))), + ); + } + #[test] fn test_decimal_coerce() { assert_eq!( diff --git a/rust/lance-index/src/scalar/expression.rs b/rust/lance-index/src/scalar/expression.rs index 5662dd4ef7e..c5fd1017836 100644 --- a/rust/lance-index/src/scalar/expression.rs +++ b/rust/lance-index/src/scalar/expression.rs @@ -2673,6 +2673,10 @@ mod tests { parser: Box, } + fn f16_scalar(value: f32) -> ScalarValue { + ScalarValue::Float16(Some(half::f16::from_f32(value))) + } + impl ColInfo { fn new(data_type: DataType, parser: Box) -> Self { Self { @@ -2934,6 +2938,57 @@ mod tests { assert!(plan.refine_expr.is_none()); } + /// A `Float16` column must reach its scalar index like any other numeric + /// column. This is a second, quieter face of the coercion gap: `maybe_scalar` + /// runs `safe_coerce_scalar` on a literal the planner already coerced, so + /// without a `Float16` arm the whole predicate silently becomes a refine + /// filter. The rows stay correct, which is why only the plan catches it. + #[rstest] + #[case("temp = 1.0", SargableQuery::Equals(f16_scalar(1.0)))] + #[case( + "temp < 1.0", + SargableQuery::Range(Bound::Unbounded, Bound::Excluded(f16_scalar(1.0))) + )] + #[case( + // Four elements so DataFusion's `ShortenInListSimplifier` leaves the list + // alone; at three or fewer over a bare column it becomes an `OR` chain and + // stops exercising `maybe_scalar_list`. + "temp IN (1.0, 2.5, 3.0, 4.0)", + SargableQuery::IsIn(vec![ + f16_scalar(1.0), + f16_scalar(2.5), + f16_scalar(3.0), + f16_scalar(4.0), + ]) + )] + fn test_float16_column_reaches_its_index(#[case] expr: &str, #[case] expected: SargableQuery) { + let index_info = MockIndexInfoProvider::new(vec![( + "temp", + ColInfo::new( + DataType::Float16, + Box::new(SargableQueryParser::new( + "temp_idx".to_string(), + "BTree".to_string(), + false, + )), + ), + )]); + let schema = Schema::new(vec![Field::new("temp", DataType::Float16, true)]); + + check_with_schema( + &index_info, + expr, + Some(IndexedExpression::index_query( + "temp".to_string(), + "temp_idx".to_string(), + "BTree".to_string(), + Arc::new(expected), + )), + true, + schema, + ); + } + #[test] fn test_expressions() { let index_info = MockIndexInfoProvider::new(vec![ diff --git a/rust/lance/tests/query/primitives.rs b/rust/lance/tests/query/primitives.rs index 608a59abb61..032c7b48f10 100644 --- a/rust/lance/tests/query/primitives.rs +++ b/rust/lance/tests/query/primitives.rs @@ -138,6 +138,7 @@ async fn test_btree_nullable_or_with_absent_value() { #[tokio::test] #[rstest::rstest] +#[case::float16(DataType::Float16)] #[case::float32(DataType::Float32)] #[case::float64(DataType::Float64)] async fn test_query_float(#[case] data_type: DataType) { @@ -146,17 +147,20 @@ async fn test_query_float(#[case] data_type: DataType) { .col("value", array::rand_type(&data_type).with_random_nulls(0.1)) .into_batch_rows(RowCount::from(60)) .unwrap(); + // BloomFilter is left out for Float16 because the index rejects the type + // outright (`Bloom filter index does not support data type: Float16`), which + // is a gap in that index rather than in literal coercion. + let mut index_types = vec![ + None, + Some(IndexType::BTree), + Some(IndexType::Bitmap), + Some(IndexType::ZoneMap), + ]; + if data_type != DataType::Float16 { + index_types.push(Some(IndexType::BloomFilter)); + } DatasetTestCases::from_data(batch) - .with_index_types( - "value", - [ - None, - Some(IndexType::BTree), - Some(IndexType::Bitmap), - Some(IndexType::BloomFilter), - Some(IndexType::ZoneMap), - ], - ) + .with_index_types("value", index_types) .run(|ds: Dataset, original: RecordBatch| async move { test_scan(&original, &ds).await; test_take(&original, &ds).await; From 923c587c2ed0841e59bae89ecd380cf2943e6911 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Fri, 28 Aug 2026 23:55:16 +0800 Subject: [PATCH 2/6] fix: make the Float16 review findings hold 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. --- rust/lance-datafusion/src/expr.rs | 59 ++++++++++++++--------- rust/lance-index/src/scalar/expression.rs | 41 ++++++++++------ rust/lance/tests/query/primitives.rs | 48 ++++++++++++++++-- 3 files changed, 105 insertions(+), 43 deletions(-) diff --git a/rust/lance-datafusion/src/expr.rs b/rust/lance-datafusion/src/expr.rs index 547cef6a623..5e3debc2edc 100644 --- a/rust/lance-datafusion/src/expr.rs +++ b/rust/lance-datafusion/src/expr.rs @@ -13,22 +13,30 @@ use half::f16; const MS_PER_DAY: i64 = 86400000; -/// Coerce a float to `f16`, rejecting a finite value that does not survive the -/// much narrower `f16` range. +/// Coerce a float to `f16`, rejecting a finite value that leaves the `f16` range +/// rather than saturating it to an infinity. /// -/// Rounding within the range is fine and matches the integer-to-float arms -/// below: a literal that lands on a nearby `f16` still filters sensibly. -/// Saturating does not, because this function cannot see the operator it is -/// coercing for. `value < 100000` would answer correctly once the literal -/// became infinity, but `value = 100000` would then match rows that really hold -/// infinity, and `value = 1e-30` collapsed to zero would match real zeros. An -/// infinite or NaN input converts faithfully and is kept. +/// A value inside the range is rounded to the nearest `f16`, which is inexact in +/// a way that shows: past 2048 the grid is coarser than the integers, so +/// `= 2049` matches rows holding 2048 and `< 65519` excludes the rows equal to +/// 65504. The `Float32` and `Float64` arms below round the same way on a finer +/// grid, and that is not what the rejection is for. /// -/// `Float64` to `Float32` saturates rather than rejecting. Reaching that takes a -/// literal above 1e38, while `f16` overflows at 65520, which ordinary data -/// passes. +/// The rejection is for the literal changing kind. Saturated to an infinity, +/// `= 100000` matches rows that really hold infinity; collapsed to zero, +/// `= 1e-30` matches real zeros. The cost is the queries where saturating would +/// have been right: `< 100000` errors instead of returning every finite row. +/// This function cannot see the operator, so it cannot allow saturation only +/// where it is harmless, and an error the caller reports beats a wrong row set. +/// An infinite or NaN input converts faithfully and is kept. +/// +/// `Float64` to `Float32` still saturates. Reaching that takes a literal above +/// 1e38, while `f16` overflows at 65520, which ordinary data passes. +/// +/// `from_f64_const` rather than `from_f64`: the latter converts through `f32` on +/// x86 with f16c and directly elsewhere, which moves that boundary by target. fn coerce_to_f16(value: f64) -> Option { - let coerced = f16::from_f64(value); + let coerced = f16::from_f64_const(value); if coerced.is_infinite() && !value.is_infinite() { return None; } @@ -936,27 +944,30 @@ mod tests { } /// Rounding inside the `f16` range is accepted; leaving the range is not. - /// Saturating to infinity or collapsing to zero would answer an ordered - /// comparison correctly and an equality wrongly, and this function cannot - /// see which operator it is coercing for. + /// Expected values are spelled as bits so a change in how the input is + /// rounded fails here rather than being recomputed by the same library call + /// the code under test uses. #[rstest::rstest] - // Rounds to the nearest representable f16 (0.1 is not exact in any binary float). - #[case::rounds(0.1, Some(0.1_f32))] - #[case::largest_finite(65504.0, Some(65504.0))] - #[case::smallest_subnormal(6e-8, Some(6e-8))] - // Overflows the f16 range rather than saturating to infinity. + // 0.1 has no exact binary form, so it lands on the nearest f16, 0x2E66. + #[case::rounds(0.1, Some(0x2E66))] + #[case::largest_finite(65504.0, Some(0x7BFF))] + // Rounds down to the largest finite f16 rather than overflowing. + #[case::just_under_overflow(65519.0, Some(0x7BFF))] + #[case::smallest_subnormal(6e-8, Some(0x0001))] + // 65520 is the first value that rounds to infinity, not 65504. + #[case::overflow_threshold(65520.0, None)] #[case::overflow(70000.0, None)] #[case::negative_overflow(-70000.0, None)] #[case::f32_max(f32::MAX as f64, None)] // Underflows to zero rather than silently matching real zeros. #[case::underflow(1e-30, None)] #[case::negative_underflow(-1e-30, None)] - fn test_f16_range_edges(#[case] input: f64, #[case] expected: Option) { + fn test_f16_range_edges(#[case] input: f64, #[case] expected: Option) { let coerced = safe_coerce_scalar(&ScalarValue::Float64(Some(input)), &DataType::Float16); match expected { - Some(expected) => assert_eq!( + Some(bits) => assert_eq!( coerced, - Some(ScalarValue::Float16(Some(f16::from_f32(expected)))), + Some(ScalarValue::Float16(Some(f16::from_bits(bits)))), ), None => assert_eq!(coerced, None), } diff --git a/rust/lance-index/src/scalar/expression.rs b/rust/lance-index/src/scalar/expression.rs index c5fd1017836..5d11f31b3dc 100644 --- a/rust/lance-index/src/scalar/expression.rs +++ b/rust/lance-index/src/scalar/expression.rs @@ -2939,12 +2939,21 @@ mod tests { } /// A `Float16` column must reach its scalar index like any other numeric - /// column. This is a second, quieter face of the coercion gap: `maybe_scalar` - /// runs `safe_coerce_scalar` on a literal the planner already coerced, so - /// without a `Float16` arm the whole predicate silently becomes a refine - /// filter. The rows stay correct, which is why only the plan catches it. + /// column. This is the second, quieter face of the coercion gap: `maybe_scalar` + /// runs `safe_coerce_scalar` on a literal the planner has *already* coerced, + /// so without a `ScalarValue::Float16` source arm the whole predicate silently + /// becomes a refine filter. The rows stay correct, which is why only the plan + /// catches it. + /// + /// This drives `Planner::parse_filter` rather than `check_with_schema` on + /// purpose. `check_with_schema` builds the expression with + /// `create_logical_expr`, which does no type coercion, so the literal would + /// still be `Float64` here and would exercise the `Float64` to `Float16` + /// target arm instead. Production coerces first, in `resolve_value`, and only + /// this order needs the source arm. #[rstest] #[case("temp = 1.0", SargableQuery::Equals(f16_scalar(1.0)))] + #[case("temp = 1", SargableQuery::Equals(f16_scalar(1.0)))] #[case( "temp < 1.0", SargableQuery::Range(Bound::Unbounded, Bound::Excluded(f16_scalar(1.0))) @@ -2975,18 +2984,20 @@ mod tests { )]); let schema = Schema::new(vec![Field::new("temp", DataType::Float16, true)]); - check_with_schema( - &index_info, - expr, - Some(IndexedExpression::index_query( - "temp".to_string(), - "temp_idx".to_string(), - "BTree".to_string(), - Arc::new(expected), - )), - true, - schema, + let planner = Planner::new(Arc::new(schema)); + let filter = planner.parse_filter(expr).unwrap(); + let plan = planner + .create_filter_plan(filter, &index_info, true) + .unwrap(); + let wanted = IndexedExpression::index_query( + "temp".to_string(), + "temp_idx".to_string(), + "BTree".to_string(), + Arc::new(expected), ); + + assert_eq!(plan.index_query, wanted.scalar_query, "predicate: {expr}"); + assert!(plan.refine_expr.is_none(), "predicate: {expr}"); } #[test] diff --git a/rust/lance/tests/query/primitives.rs b/rust/lance/tests/query/primitives.rs index 032c7b48f10..07417ba637c 100644 --- a/rust/lance/tests/query/primitives.rs +++ b/rust/lance/tests/query/primitives.rs @@ -6,7 +6,8 @@ use std::sync::Arc; use arrow::datatypes::*; use arrow_array::{ ArrayRef, BinaryArray, BinaryViewArray, Float32Array, Float64Array, Int32Array, - LargeBinaryArray, LargeStringArray, RecordBatch, StringArray, StringViewArray, + LargeBinaryArray, LargeStringArray, RecordBatch, RecordBatchIterator, StringArray, + StringViewArray, }; use arrow_schema::DataType; use lance::Dataset; @@ -16,6 +17,7 @@ use lance::dataset::{InsertBuilder, WriteParams}; use lance::index::DatasetIndexExt; use lance_datagen::{ArrayGeneratorExt, RowCount, array, gen_batch}; use lance_index::IndexType; +use lance_index::scalar::ScalarIndexParams; use super::{test_filter, test_scan, test_take}; use crate::utils::DatasetTestCases; @@ -147,9 +149,9 @@ async fn test_query_float(#[case] data_type: DataType) { .col("value", array::rand_type(&data_type).with_random_nulls(0.1)) .into_batch_rows(RowCount::from(60)) .unwrap(); - // BloomFilter is left out for Float16 because the index rejects the type - // outright (`Bloom filter index does not support data type: Float16`), which - // is a gap in that index rather than in literal coercion. + // BloomFilter is left out for Float16 because that index rejects the type + // outright. `test_bloom_filter_rejects_float16` pins the rejection so this + // skip cannot outlive it. let mut index_types = vec![ None, Some(IndexType::BTree), @@ -174,6 +176,44 @@ async fn test_query_float(#[case] data_type: DataType) { .await } +/// `test_query_float` runs its Float16 case without `IndexType::BloomFilter` +/// because that index refuses the type. Pin the refusal here so the skip cannot +/// outlive it: once bloom filters accept Float16 this test fails, and whoever +/// makes it pass should drop the skip too. +#[tokio::test] +async fn test_bloom_filter_rejects_float16() { + let batch = gen_batch() + .col("value", array::rand_type(&DataType::Float16)) + .into_batch_rows(RowCount::from(16)) + .unwrap(); + let mut ds = Dataset::write( + RecordBatchIterator::new(vec![Ok(batch.clone())], batch.schema()), + "memory://bloom_f16", + None, + ) + .await + .unwrap(); + + let err = ds + .create_index( + &["value"], + IndexType::BloomFilter, + None, + &ScalarIndexParams::default(), + false, + ) + .await + .expect_err("bloom filter should still refuse Float16"); + assert!( + matches!(err, lance::Error::InvalidInput { .. }), + "unexpected error variant: {err:?}" + ); + assert!( + err.to_string().contains("Float16"), + "error should name the rejected type: {err}" + ); +} + #[tokio::test] #[rstest::rstest] #[case::float32(DataType::Float32)] From 0dffbd98f70e85f06197b84f87f5252ebd70316e Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sat, 29 Aug 2026 00:11:24 +0800 Subject: [PATCH 3/6] docs: state what the f16 conversion actually drops The previous wording said the truncation was unreachable from a written literal. It is reachable: = 2049.001 matches rows holding 2048. --- rust/lance-datafusion/src/expr.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/rust/lance-datafusion/src/expr.rs b/rust/lance-datafusion/src/expr.rs index 5e3debc2edc..08dfa556b05 100644 --- a/rust/lance-datafusion/src/expr.rs +++ b/rust/lance-datafusion/src/expr.rs @@ -16,7 +16,7 @@ const MS_PER_DAY: i64 = 86400000; /// Coerce a float to `f16`, rejecting a finite value that leaves the `f16` range /// rather than saturating it to an infinity. /// -/// A value inside the range is rounded to the nearest `f16`, which is inexact in +/// A value inside the range is rounded onto the `f16` grid, which is inexact in /// a way that shows: past 2048 the grid is coarser than the integers, so /// `= 2049` matches rows holding 2048 and `< 65519` excludes the rows equal to /// 65504. The `Float32` and `Float64` arms below round the same way on a finer @@ -31,10 +31,13 @@ const MS_PER_DAY: i64 = 86400000; /// An infinite or NaN input converts faithfully and is kept. /// /// `Float64` to `Float32` still saturates. Reaching that takes a literal above -/// 1e38, while `f16` overflows at 65520, which ordinary data passes. +/// 1e38, while `f16` overflows at 65520, which an ordinary literal passes. /// /// `from_f64_const` rather than `from_f64`: the latter converts through `f32` on /// x86 with f16c and directly elsewhere, which moves that boundary by target. +/// The const form drops the low 32 bits of the `f64` mantissa before rounding, so +/// 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 { let coerced = f16::from_f64_const(value); if coerced.is_infinite() && !value.is_infinite() { @@ -950,6 +953,14 @@ mod tests { #[rstest::rstest] // 0.1 has no exact binary form, so it lands on the nearest f16, 0x2E66. #[case::rounds(0.1, Some(0x2E66))] + // Past 2048 the f16 grid is coarser than the integers: 2049 is the exact + // midpoint of 2048 and 2050, and the tie goes to the even mantissa. This is + // the case the doc comment cites for `= 2049` matching rows holding 2048. + #[case::odd_integer_ties_down(2049.0, Some(0x6800))] + // `from_f64_const` discards the low 32 bits of the f64 mantissa, so anything + // in [2049, 2049.001953125) is treated as that same tie even though 2050 is + // nearer. Reachable from a written literal, which is why the doc says so. + #[case::truncated_tie(2049.001, Some(0x6800))] #[case::largest_finite(65504.0, Some(0x7BFF))] // Rounds down to the largest finite f16 rather than overflowing. #[case::just_under_overflow(65519.0, Some(0x7BFF))] From 205b25c01d10c0218759c57af43385985ff6dcd8 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sat, 29 Aug 2026 01:23:55 +0800 Subject: [PATCH 4/6] docs: let the tests carry the f16 rounding detail 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. --- rust/lance-datafusion/src/expr.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/rust/lance-datafusion/src/expr.rs b/rust/lance-datafusion/src/expr.rs index 08dfa556b05..bf2913ea9a5 100644 --- a/rust/lance-datafusion/src/expr.rs +++ b/rust/lance-datafusion/src/expr.rs @@ -16,11 +16,11 @@ const MS_PER_DAY: i64 = 86400000; /// Coerce a float to `f16`, rejecting a finite value that leaves the `f16` range /// rather than saturating it to an infinity. /// -/// A value inside the range is rounded onto the `f16` grid, which is inexact in -/// a way that shows: past 2048 the grid is coarser than the integers, so -/// `= 2049` matches rows holding 2048 and `< 65519` excludes the rows equal to -/// 65504. The `Float32` and `Float64` arms below round the same way on a finer -/// grid, and that is not what the rejection is for. +/// A value inside the range lands on the `f16` grid and is inexact in a way that +/// shows: past 2048 the grid is coarser than the integers, so `= 2049` matches +/// rows holding 2048 and `< 65519` excludes the rows equal to 65504. The +/// `Float32` and `Float64` arms below are inexact too, on a finer grid, and that +/// is not what the rejection is for. /// /// The rejection is for the literal changing kind. Saturated to an infinity, /// `= 100000` matches rows that really hold infinity; collapsed to zero, @@ -35,9 +35,8 @@ const MS_PER_DAY: i64 = 86400000; /// /// `from_f64_const` rather than `from_f64`: the latter converts through `f32` on /// x86 with f16c and directly elsewhere, which moves that boundary by target. -/// The const form drops the low 32 bits of the `f64` mantissa before rounding, so -/// 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. +/// It is also not exactly round-to-nearest; `test_f16_range_edges` pins where it +/// lands. fn coerce_to_f16(value: f64) -> Option { let coerced = f16::from_f64_const(value); if coerced.is_infinite() && !value.is_infinite() { @@ -957,9 +956,9 @@ mod tests { // midpoint of 2048 and 2050, and the tie goes to the even mantissa. This is // the case the doc comment cites for `= 2049` matching rows holding 2048. #[case::odd_integer_ties_down(2049.0, Some(0x6800))] - // `from_f64_const` discards the low 32 bits of the f64 mantissa, so anything - // in [2049, 2049.001953125) is treated as that same tie even though 2050 is - // nearer. Reachable from a written literal, which is why the doc says so. + // Above that midpoint 2050 is the nearer neighbour, and this still lands on + // 2048, because `from_f64_const` truncates before it rounds. A written + // literal reaches it, so it is not a theoretical gap. #[case::truncated_tie(2049.001, Some(0x6800))] #[case::largest_finite(65504.0, Some(0x7BFF))] // Rounds down to the largest finite f16 rather than overflowing. From a6362940a75cc8b7cef257d50249c05cd5eb2d77 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sat, 29 Aug 2026 01:32:46 +0800 Subject: [PATCH 5/6] docs: name the subject so the sentence cannot read as from_f64 from_f64 is correctly rounded on aarch64 with fp16, so the claim is only true of the const form. --- rust/lance-datafusion/src/expr.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/lance-datafusion/src/expr.rs b/rust/lance-datafusion/src/expr.rs index bf2913ea9a5..5c14c9bc29b 100644 --- a/rust/lance-datafusion/src/expr.rs +++ b/rust/lance-datafusion/src/expr.rs @@ -35,8 +35,8 @@ const MS_PER_DAY: i64 = 86400000; /// /// `from_f64_const` rather than `from_f64`: the latter converts through `f32` on /// x86 with f16c and directly elsewhere, which moves that boundary by target. -/// It is also not exactly round-to-nearest; `test_f16_range_edges` pins where it -/// lands. +/// The const form is also not exactly round-to-nearest; `test_f16_range_edges` +/// pins where it lands. fn coerce_to_f16(value: f64) -> Option { let coerced = f16::from_f64_const(value); if coerced.is_infinite() && !value.is_infinite() { From 6e0ed0135d8d3afc74b6682eaeeaff457b72da6e Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sat, 29 Aug 2026 03:13:00 +0800 Subject: [PATCH 6/6] fix(datafusion): round f64 to f16 correctly 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. --- rust/lance-datafusion/src/expr.rs | 156 +++++++++++++++++++++++++++--- 1 file changed, 140 insertions(+), 16 deletions(-) diff --git a/rust/lance-datafusion/src/expr.rs b/rust/lance-datafusion/src/expr.rs index 5c14c9bc29b..6c9743a8eab 100644 --- a/rust/lance-datafusion/src/expr.rs +++ b/rust/lance-datafusion/src/expr.rs @@ -13,14 +13,60 @@ use half::f16; const MS_PER_DAY: i64 = 86400000; +/// The exact tie between the largest finite `f16` and the value that would +/// follow it. A tie goes to the even mantissa, which there is the one that does +/// not exist, so this magnitude and anything above it rounds to an infinity. +const F16_OVERFLOW_THRESHOLD: f64 = 65520.0; + +/// The finite `f16` nearest `value`, with a tie going to the even mantissa. +/// +/// Neither of `half`'s conversions is correctly rounded, in two different ways: +/// the software path truncates the low 32 bits of the `f64` mantissa before it +/// rounds ([half-rs#151]), and the x86 hardware path rounds through `f32` first +/// ([half-rs#116]). Both land within one step of the right answer, so this starts +/// from the software path, which at least does not vary by target, and then looks +/// at the two neighbours. Widening `f16` to `f64` is exact, so comparing the three +/// distances in `f64` decides exactly. +/// +/// The caller must have excluded the overflow range first. Truncation only ever +/// shrinks the magnitude, so below the threshold the starting point is finite. +/// +/// [half-rs#151]: https://github.com/VoidStarKat/half-rs/issues/151 +/// [half-rs#116]: https://github.com/VoidStarKat/half-rs/issues/116 +fn nearest_finite_f16(value: f64) -> f16 { + let start = f16::from_f64_const(value); + debug_assert!( + start.is_finite(), + "caller must reject the overflow range before calling: {value}" + ); + let mut best = start; + let mut best_distance = (value - start.to_f64()).abs(); + let start_bits = start.to_bits(); + // Stepping the bit pattern by one walks to the adjacent magnitude on either + // side of `start`, for negatives as well. Stepping off an end lands on an + // infinity or a NaN, which is not a candidate. + for candidate in [start_bits.wrapping_sub(1), start_bits.wrapping_add(1)].map(f16::from_bits) { + if !candidate.is_finite() { + continue; + } + let distance = (value - candidate.to_f64()).abs(); + let wins = + distance < best_distance || (distance == best_distance && candidate.to_bits() & 1 == 0); + if wins { + best = candidate; + best_distance = distance; + } + } + best +} + /// Coerce a float to `f16`, rejecting a finite value that leaves the `f16` range /// rather than saturating it to an infinity. /// /// A value inside the range lands on the `f16` grid and is inexact in a way that /// shows: past 2048 the grid is coarser than the integers, so `= 2049` matches -/// rows holding 2048 and `< 65519` excludes the rows equal to 65504. The -/// `Float32` and `Float64` arms below are inexact too, on a finer grid, and that -/// is not what the rejection is for. +/// rows holding 2048. The `Float32` and `Float64` arms below are inexact too, on +/// a finer grid, and that is not what the rejection is for. /// /// The rejection is for the literal changing kind. Saturated to an infinity, /// `= 100000` matches rows that really hold infinity; collapsed to zero, @@ -32,20 +78,27 @@ const MS_PER_DAY: i64 = 86400000; /// /// `Float64` to `Float32` still saturates. Reaching that takes a literal above /// 1e38, while `f16` overflows at 65520, which an ordinary literal passes. -/// -/// `from_f64_const` rather than `from_f64`: the latter converts through `f32` on -/// x86 with f16c and directly elsewhere, which moves that boundary by target. -/// The const form is also not exactly round-to-nearest; `test_f16_range_edges` -/// pins where it lands. fn coerce_to_f16(value: f64) -> Option { - let coerced = f16::from_f64_const(value); - if coerced.is_infinite() && !value.is_infinite() { + if value.is_nan() { + return Some(f16::NAN); + } + if value.is_infinite() { + return Some(if value.is_sign_positive() { + f16::INFINITY + } else { + f16::NEG_INFINITY + }); + } + if value.abs() >= F16_OVERFLOW_THRESHOLD { return None; } - if coerced == f16::ZERO && value != 0.0 { + let nearest = nearest_finite_f16(value); + // `f16::ZERO == f16::NEG_ZERO`, and so does `-0.0 == 0.0`, so a signed zero + // literal keeps its sign here and only a genuinely nonzero value is rejected. + if nearest == f16::ZERO && value != 0.0 { return None; } - Some(coerced) + Some(nearest) } // This is slightly tedious but when we convert expressions from SQL strings to logical @@ -956,10 +1009,10 @@ mod tests { // midpoint of 2048 and 2050, and the tie goes to the even mantissa. This is // the case the doc comment cites for `= 2049` matching rows holding 2048. #[case::odd_integer_ties_down(2049.0, Some(0x6800))] - // Above that midpoint 2050 is the nearer neighbour, and this still lands on - // 2048, because `from_f64_const` truncates before it rounds. A written - // literal reaches it, so it is not a theoretical gap. - #[case::truncated_tie(2049.001, Some(0x6800))] + // Above that midpoint 2050 is the nearer neighbour, and the coercion picks it. + // `half`'s own conversion returns 2048 here, which is the defect + // `nearest_finite_f16` exists to correct. + #[case::just_above_a_tie(2049.001, Some(0x6801))] #[case::largest_finite(65504.0, Some(0x7BFF))] // Rounds down to the largest finite f16 rather than overflowing. #[case::just_under_overflow(65519.0, Some(0x7BFF))] @@ -1018,6 +1071,77 @@ mod tests { ); } + /// Sweep every rounding decision the conversion can make instead of trusting + /// the handful of points named above: for each adjacent pair of finite `f16` + /// values, the exact midpoint and the two `f64` values either side of it. A + /// misrounding anywhere in the range shows up here, which is how the + /// `2049.001` case was found in the first place. + /// + /// The expectation is stated, not recomputed: below the midpoint the lower + /// neighbour, above it the upper one, at it the even mantissa. + #[test] + fn test_f16_rounds_to_nearest_even_across_the_whole_range() { + fn coerce(value: f64) -> Option { + match safe_coerce_scalar(&ScalarValue::Float64(Some(value)), &DataType::Float16) { + Some(ScalarValue::Float16(Some(v))) => Some(v), + // Rejected, which the underflow side of the range expects. + None => None, + other => panic!("expected a Float16 literal for {value}, got {other:?}"), + } + } + // A nonzero literal that lands on zero is rejected rather than coerced, + // so the smallest pair expects `None` on its lower side. + fn want(expected: f16, input: f64) -> Option { + if expected == f16::ZERO && input != 0.0 { + None + } else { + Some(expected) + } + } + + // 0x7BFF is the largest finite f16, so pairing each bit pattern with the + // next covers every adjacent finite pair on the positive side. Negatives + // are covered by the symmetry check below. + for lower_bits in 0..0x7BFFu16 { + let lower = f16::from_bits(lower_bits); + let upper = f16::from_bits(lower_bits + 1); + // Both operands are f16 widened to f64, so the average is exact. + let midpoint = (lower.to_f64() + upper.to_f64()) / 2.0; + let below = f64::from_bits(midpoint.to_bits() - 1); + let above = f64::from_bits(midpoint.to_bits() + 1); + + assert_eq!(coerce(below), want(lower, below), "just below {midpoint}"); + assert_eq!(coerce(above), want(upper, above), "just above {midpoint}"); + let even = if lower.to_bits() & 1 == 0 { + lower + } else { + upper + }; + assert_eq!(coerce(midpoint), want(even, midpoint), "at {midpoint}"); + } + } + + /// Sign is not part of the rounding decision, so negating the input negates + /// the result. This is what lets the sweep above cover only positives. + #[rstest::rstest] + #[case(0.1)] + #[case(2049.001)] + #[case(65504.0)] + #[case(6e-8)] + #[case(70000.0)] + #[case(1e-30)] + fn test_f16_coercion_is_sign_symmetric(#[case] magnitude: f64) { + let positive = + safe_coerce_scalar(&ScalarValue::Float64(Some(magnitude)), &DataType::Float16); + let negative = + safe_coerce_scalar(&ScalarValue::Float64(Some(-magnitude)), &DataType::Float16); + let flipped = match positive { + Some(ScalarValue::Float16(Some(v))) => Some(ScalarValue::Float16(Some(-v))), + other => other, + }; + assert_eq!(negative, flipped); + } + #[test] fn test_decimal_coerce() { assert_eq!(