github-actions[bot] commented on code in PR #68128: URL: https://github.com/apache/doris/pull/68128#discussion_r4092851242
########## fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergPredicateConverterFloatSemanticsTest.java: ########## @@ -0,0 +1,456 @@ +// 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. + +package org.apache.doris.connector.iceberg; + +import org.apache.doris.connector.spi.ConnectorType; +import org.apache.doris.connector.spi.pushdown.ConnectorColumnRef; +import org.apache.doris.connector.spi.pushdown.ConnectorComparison; +import org.apache.doris.connector.spi.pushdown.ConnectorExpression; +import org.apache.doris.connector.spi.pushdown.ConnectorIn; +import org.apache.doris.connector.spi.pushdown.ConnectorLiteral; +import org.apache.doris.connector.spi.pushdown.ConnectorNot; + +import org.apache.iceberg.DataFile; +import org.apache.iceberg.DataFiles; +import org.apache.iceberg.FileFormat; +import org.apache.iceberg.Metrics; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.Schema; +import org.apache.iceberg.StructLike; +import org.apache.iceberg.expressions.And; +import org.apache.iceberg.expressions.Evaluator; +import org.apache.iceberg.expressions.Expression; +import org.apache.iceberg.expressions.InclusiveMetricsEvaluator; +import org.apache.iceberg.expressions.Or; +import org.apache.iceberg.types.Conversions; +import org.apache.iceberg.types.Types; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.nio.ByteBuffer; +import java.time.ZoneOffset; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * DORIS-29047 (NaN) and its signed-zero sibling: a FLOAT/DOUBLE predicate pushed to iceberg must not prune a + * file that holds rows Doris considers matching. A pruned file never becomes a split, so BE never sees those + * rows and cannot filter them back in — the query silently returns too few rows rather than failing. + * + * <p>Two oracles, because either one alone misses the bug: + * <ul> + * <li>{@link InclusiveMetricsEvaluator} over hand-built file metrics — the actual pruning decision, and a + * direct reproduction of the reported queries (metrics shaped exactly as spark / iceberg-java write + * them: NaN never reaches the bounds, {@code -0.0} stays {@code -0.0}).</li> + * <li>The row-level {@link Evaluator} over the same converted expression — pins that the expression is + * EQUIVALENT to the Doris predicate rather than merely wider. Equivalence is what makes NOT / AND / OR + * compose: iceberg's RewriteNot turns {@code not(or(gt, isNaN))} into {@code and(ltEq, notNaN)}, which + * is exactly Doris's {@code NOT(d > v)} over a NaN row.</li> + * </ul> + */ +public class IcebergPredicateConverterFloatSemanticsTest { + + private static final int D_ID = 1; + + private static final Schema SCHEMA = new Schema( + Types.NestedField.optional(D_ID, "d", Types.DoubleType.get()), + Types.NestedField.optional(2, "f", Types.FloatType.get()), + Types.NestedField.optional(3, "i", Types.IntegerType.get())); + + // The iceberg spec forbids NaN as a bound ("NaNs are not permitted as lower or upper bounds"), so a NaN + // shows up only in nan_value_counts; -0.0 survives in the bounds because they use the IEEE total order. + private static final DataFile NAN_ONLY = file("nan_only", 1, 1L, null, null); + private static final DataFile NAN_MIXED = file("nan_mixed", 2, 1L, 1.0d, 1.0d); + private static final DataFile NEG_ZERO = file("negzero", 1, 0L, -0.0d, -0.0d); + private static final DataFile POS_ZERO = file("poszero", 1, 0L, 0.0d, 0.0d); + private static final DataFile PLAIN = file("plain", 2, 0L, 10.0d, 20.0d); + // Doris's own writer reports no NaN count at all (IcebergWriterHelper passes a null nanValueCounts), so + // nothing in the metadata can rule NaN out and such a file must survive every float range predicate. + private static final DataFile UNKNOWN_NAN = file("doris_written", 2, null, 1.0d, 1.0d); + // Same bounds as NAN_MIXED and UNKNOWN_NAN, but the writer states there is no NaN -- the only difference + // that may bring pruning back. + private static final DataFile NO_NAN_ONE_VALUE = file("one_value", 2, 0L, 1.0d, 1.0d); + // Every value is NULL: no bounds, no NaN. Doris matches no row of it for any comparison. + private static final DataFile NULL_ONLY = file("null_only", 2, 2L, 0L, null, null); + + /** + * The reported query: {@code WHERE d > 0} / {@code d >= 0} returned nothing over a single NaN row. + * + * <p>The four cases here are the deterministic counterpart of the {@code float_prune_nan_only} + * assertions in the {@code test_iceberg_float_predicate_pushdown} regression suite, whose fixture + * carries exactly these metrics. + */ + @Test + public void nanOnlyFileSurvivesRangePredicates() { + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.GT, 0.0d)), NAN_ONLY)); + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.GE, 0.0d)), NAN_ONLY)); + // NaN satisfies neither, so the file is still pruned. + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.LT, 0.0d)), NAN_ONLY)); + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.EQ, 0.0d)), NAN_ONLY)); + } + + /** + * The half that a "whole file is NaN" special case would miss: NaN hides outside the bounds, so a file + * holding {1.0, NaN} is pruned by the bounds alone for {@code d > 5}. + */ + @Test + public void nanMixedFileSurvivesRangePredicateAboveItsBounds() { + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.GT, 5.0d)), NAN_MIXED)); + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.GE, 5.0d)), NAN_MIXED)); + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.GT, 5.0d)), UNKNOWN_NAN)); + } + + /** + * {@code !=} / {@code NOT IN} prune through {@code uniqueValue}, whose NaN guard only fires when the file + * actually reports a NaN count — so a file written without one is pruned as if its single bound value + * were its only value. + */ + @Test + public void notEqualAndNotInSurviveOnFilesThatMayHoldNaN() { + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.NE, 1.0d)), UNKNOWN_NAN)); + Assertions.assertTrue(mayMatch(single(notIn("d", 1.0d)), UNKNOWN_NAN)); + } + + /** + * The fix must not blanket-disable pruning: a file that reports zero NaNs is still pruned, which is what + * keeps spark / flink / iceberg-java written tables fully prunable. + */ + @Test + public void rangePredicatesStillPruneFilesWithoutNaN() { + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.GT, 100.0d)), PLAIN)); + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.GE, 100.0d)), PLAIN)); + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.LT, 1.0d)), PLAIN)); + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.EQ, 1.0d)), PLAIN)); + Assertions.assertFalse(mayMatch(single(notIn("d", 1.0d)), NO_NAN_ONE_VALUE)); + Assertions.assertFalse( + mayMatch(single(cmp("d", ConnectorComparison.Operator.NE, 1.0d)), NO_NAN_ONE_VALUE)); + } + + /** + * Signed zero: Doris reads {@code -0.0 == 0.0} (IEEE) while iceberg orders {@code -0.0} strictly before + * {@code +0.0}, so a bound at the wrong zero drops the other one in both directions. + */ + @Test + public void signedZeroFilesSurviveZeroPredicates() { + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.EQ, 0.0d)), NEG_ZERO)); + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.GE, 0.0d)), NEG_ZERO)); + Assertions.assertTrue(mayMatch(single(in("d", 0.0d)), NEG_ZERO)); + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.EQ, -0.0d)), POS_ZERO)); + Assertions.assertTrue(mayMatch(single(cmp("d", ConnectorComparison.Operator.LE, -0.0d)), POS_ZERO)); + // Still precise: widening the bound to cover both zeros must not start keeping unrelated files, and + // -0.0 is neither > 0 nor < 0, so both still prune its file (the regression suite asserts the same + // four outcomes over the identically-shaped float_prune_negzero fixture). + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.GT, 0.0d)), NEG_ZERO)); + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.LT, 0.0d)), NEG_ZERO)); + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.EQ, 0.0d)), PLAIN)); + Assertions.assertFalse(mayMatch(single(cmp("d", ConnectorComparison.Operator.LE, -0.0d)), PLAIN)); + } + + /** + * {@code WHERE d > 0} carries an INT literal, not a double one (Nereids types the bare {@code 0} as an + * integer), so the zero handling has to see through the literal's Java type. + */ + @Test + public void integerZeroLiteralOnFloatingColumnIsStillAZero() { + Expression ge = single(new ConnectorComparison(ConnectorComparison.Operator.GE, col("d"), + new ConnectorLiteral(ConnectorType.of("INT"), 0L))); + Assertions.assertTrue(mayMatch(ge, NEG_ZERO), "d >= 0 must keep a -0.0-only file"); + Assertions.assertTrue(mayMatch(ge, NAN_ONLY), "d >= 0 must keep a NaN-only file"); + } + + /** A FLOAT column takes the same path — the reconciliation keys off the iceberg column type. */ + @Test + public void floatColumnGetsTheSameTreatment() { + Expression gt = single(new ConnectorComparison(ConnectorComparison.Operator.GT, + col("f"), new ConnectorLiteral(ConnectorType.of("FLOAT"), 0.0d))); + Assertions.assertEquals(Expression.Operation.OR, gt.op()); + Assertions.assertEquals(Expression.Operation.IS_NAN, ((Or) gt).right().op()); + } + + /** + * A NaN literal is reachable ({@code WHERE d > cast('nan' as double)} — Nereids' DoubleLiteral parses the Review Comment: [P2] Exercise the actual typed-literal rewrite path This test bypasses the producer for the advertised `WHERE d > CAST('NaN' AS DOUBLE)` case: parsing leaves the RHS as a `Cast`, distributed EXECUTE forwards that unanalysed WHERE to `UnboundExpressionToConnectorPredicateConverter`, and that converter only accepts a comparison operand that is already a `Literal`. The real rewrite SQL therefore fails as unsupported before `IcebergPredicateConverter` sees the NaN; meanwhile `cmp()` constructs a `ConnectorLiteral` directly and `single()` uses SCAN mode. Please lower/fold typed literal casts before neutral conversion and cover the parser/EXECUTE-to-REWRITE chain. -- 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]
