comphead commented on code in PR #6447: URL: https://github.com/apache/datafusion-comet/pull/6447#discussion_r4146865173
########## spark/src/test/resources/sql-tests/expressions/conditional/float_comparisons.sql: ########## @@ -0,0 +1,140 @@ +-- Licensed to the Apache Software Foundation (ASF) under one +-- or more contributor license agreements. See the NOTICE file +-- distributed with this work for additional information +-- regarding copyright ownership. The ASF licenses this file +-- to you under the Apache License, Version 2.0 (the +-- "License"); you may not use this file except in compliance +-- with the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, +-- software distributed under the License is distributed on an +-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +-- KIND, either express or implied. See the License for the +-- specific language governing permissions and limitations +-- under the License. + +-- Comparisons follow Spark's SQL ordering for floats (SQLOrderingUtil.compareDoubles): -0.0 +-- equals 0.0, every NaN equals every other NaN, and NaN sorts above every other value. This +-- must hold in every operator that evaluates a comparison, not only in Project and Filter. +-- +-- `-d` flips the sign bit, so `-d` of the NaN row is a NaN with the sign bit set on every +-- platform. That is the NaN that arithmetic produces on x86-64, and Arrow's total order sorts +-- it below -Infinity. + +statement +CREATE TABLE float_cmp(id INT, d DOUBLE, f FLOAT) USING parquet + +statement +INSERT INTO float_cmp VALUES + (1, 0.0D, float('0.0')), + (2, double('-0.0'), float('-0.0')), + (3, double('NaN'), float('NaN')), + (4, 1.0D, float('1.0')), + (5, -1.0D, float('-1.0')), + (6, double('Infinity'), float('Infinity')), + (7, NULL, NULL) + +-- Project +query +SELECT id, -d = d, -d <=> d, -d < d, -d <= d, -d > d, -d >= d, -d != d FROM float_cmp + +query +SELECT id, -f = f, -f <=> f, -f < f, -f <= f, -f > f, -f >= f, -f != f FROM float_cmp + +-- Comparisons against a constant on either side. `-0.0D` and `-0.0F` are literals with the sign +-- bit set, while `double('NaN')` is a cast, because the suite turns off constant folding. +query +SELECT id, -d > 0.0D, -d >= -0.0D, -0.0D = d, -d = double('NaN'), double('NaN') <= -d, + -d < double('-0.0'), -f > 0.0F, -0.0F <=> f, -f = float('NaN'), float('-0.0') >= -f +FROM float_cmp + +-- Filter +query +SELECT id FROM float_cmp WHERE -d > 0.0D Review Comment: Every `WHERE` on a float column here compares a computed operand (`-d` or `-f`), so none has the `column op literal` shape that a native scan pushes down as a data filter. Would it make sense to add a few filters on the stored edge values, for example `WHERE d >= 0.0D`, `WHERE d = double('NaN')` and `WHERE d > double('Infinity')`? I haven't run them, but I'd expect Spark to return the `-0.0` and NaN rows. They would show that the Filter above the scan still applies Spark's ordering after the raw data filters. ########## native/core/src/execution/planner.rs: ########## @@ -1007,6 +1017,23 @@ impl PhysicalPlanner { } } + /// Create a data filter that a scan pushes into the Parquet reader. The filter prunes row + /// groups and pages, and rows too when row-level pushdown is enabled. Pruning only recognizes + /// a column compared with a literal, so the filter's comparisons leave float operands as they + /// are rather than normalizing them. Spark's Filter above the scan evaluates the filter again + /// with Spark's semantics. + fn create_data_filter( + &self, + spark_expr: &Expr, + input_schema: SchemaRef, + ) -> Result<Arc<dyn PhysicalExpr>, ExecutionError> { + let planner = Self { + float_operands: FloatOperands::Raw, Review Comment: `Raw` applies to every comparison inside a data filter, while the description says pruning only recognizes a column compared with a literal. Would it make sense to leave only that shape raw and normalize the rest? I haven't run this, but with `spark.comet.parquet.rowFilterPushdown.enabled=true` I'd expect `WHERE -d > 0.0D` from `float_comparisons.sql` to lose the NaN row, because the reader would compare the computed sign-bit NaN raw. If `Raw` stays as is, it might be worth saying in the docs that computed operands are affected too. ########## native/spark-expr/src/float_semantics/normalize.rs: ########## @@ -240,6 +273,16 @@ pub fn has_float_leaf(dt: &DataType) -> bool { } } +fn is_nested_with_float_leaf(dt: &DataType) -> bool { Review Comment: `needs_spark_equality` in `nested_comparison.rs` already tests the same thing (`dt.is_nested() && has_float_leaf(dt)`). Would it be simpler to keep one `pub(crate)` helper and use it from both files? ########## native/spark-expr/src/float_semantics/normalize.rs: ########## @@ -87,15 +88,17 @@ impl PhysicalExpr for NormalizeNaNAndZero { } fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> { - let cv = self.child.evaluate(batch)?; - let array = cv.into_array(batch.num_rows())?; - match &self.data_type { - DataType::Float32 | DataType::Float64 => { - Ok(ColumnarValue::Array(normalize_floats(&array))) - } + DataType::Float32 | DataType::Float64 => {} dt => panic!("Unexpected data type {dt:?}"), } + // A scalar stays a scalar, so a constant operand is not expanded into a column. + match self.child.evaluate(batch)? { + ColumnarValue::Array(array) => Ok(ColumnarValue::Array(normalize_floats(&array))), + ColumnarValue::Scalar(value) => { Review Comment: Since comparisons outside Project and Filter now pay for this, would it make sense to give `normalize_floats` a read-only fast path? DataFusion's `normalize_float_zero`, which its `apply_cmp` uses, scans first and returns the input `Arc` unless a `-0.0` is present. Here `unary(normalize_float)` always writes a new buffer. I haven't measured it, but I'd expect skipping that allocation in the common case to recover part of the 7.2 to 11.0 µs gap. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
