github-actions[bot] commented on code in PR #68128:
URL: https://github.com/apache/doris/pull/68128#discussion_r4082475958
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPredicateConverter.java:
##########
@@ -289,7 +283,175 @@ private Expression buildIn(ConnectorIn in) {
}
values.add(value);
}
- return in.isNegated() ? Expressions.notIn(colName, values) :
Expressions.in(colName, values);
+ return buildLeafIn(field.type(), colName, values, in.isNegated());
+ }
+
+ //
════════════════════════════════════════════════════════════════════════════════════════════════════
+ // FLOAT/DOUBLE leaves: Doris row semantics vs the total order iceberg
prunes files by.
+ //
+ // Doris evaluates a ROW with "NaN is greater than everything, NaN = NaN"
plus IEEE zero equality
+ // (-0.0 == 0.0). Iceberg prunes a FILE with Comparators.naturalOrder()
(Double.compare), where NaN is
+ // not in the bounds at all -- the spec says "NaNs are not permitted as
lower or upper bounds", they are
+ // recorded separately in nan_value_counts -- and -0.0 sorts strictly
before +0.0. Handing iceberg a
+ // literal translation therefore prunes files that do hold matching rows,
and a pruned file never becomes
+ // a split, so BE's residual filter cannot recover those rows: the query
silently returns too few rows.
+ //
+ // The leaves below do not merely widen, they emit the expression
EQUIVALENT to the Doris predicate under
+ // iceberg's order, and they spell out the NaN half on BOTH sides: a
comparison Doris matches NaN with gets
+ // `OR isNaN`, one it does not gets `AND notNaN`. That symmetry is
load-bearing, not decoration -- De Morgan
+ // maps each form onto the other, so `NOT` composes at any nesting depth.
Leaving the `AND notNaN` off would
+ // still read correctly row by row, yet iceberg's RewriteNot would lower
not(lessThan(d, 5)) to a BARE
+ // gtEq(d, 5) whose file-level evaluator prunes a {1.0, NaN} file -- the
very bug, back through the negation.
+ // notNaN costs no pruning either: it only rules out a file that is
entirely NaN.
+ //
+ // Cost: InclusiveMetricsEvaluator.isNaN only prunes a file that reports
nan_value_count == 0, so files
+ // whose metrics omit NaN counts -- including every file Doris writes
today, IcebergWriterHelper passes a
+ // null nanValueCounts -- stop being pruned by a float range predicate.
This is the trade parquet-java and
+ // parquet-cpp already make ("stats may hold NaN -> do not prune");
writers that do report NaN counts
+ // (spark, flink, iceberg-java) keep pruning in full.
+ //
════════════════════════════════════════════════════════════════════════════════════════════════════
+
+ /**
+ * {@code col OP value}, shared by all three modes. A non-floating column
gets the plain 1:1 mapping; a
+ * FLOAT/DOUBLE column gets the NaN / signed-zero reconciliation described
above.
+ */
+ private static Expression buildLeafComparison(Type type, String colName,
+ ConnectorComparison.Operator op, Object value) {
+ if (!isFloating(type)) {
+ return buildPlainComparison(colName, op, value);
+ }
+ // On a floating column extractIcebergLiteral only ever yields a
Number: Double/Float from a float
+ // literal, Double from a decimal literal, Integer/Long from an
integer one (`d > 0`).
+ double v = ((Number) value).doubleValue();
+ if (Double.isNaN(v)) {
+ // Expressions.*(col, NaN) throws ("Cannot create expression
literal from NaN") -- iceberg models a
+ // NaN literal only through the unary isNaN/notNaN. Doris: NaN is
the greatest value, NaN = NaN.
+ switch (op) {
+ case EQ:
+ case EQ_FOR_NULL:
+ case GE:
+ return Expressions.isNaN(colName);
+ case NE:
+ case LT:
+ return Expressions.notNaN(colName);
Review Comment:
[P2] Keep NULL out of NaN-negative rewrite predicates
These branches are not equivalent on nullable columns. [Iceberg 1.11.0
evaluates `notNaN(null)` as
true](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/api/src/main/java/org/apache/iceberg/expressions/Evaluator.java#L100-L102),
whereas Doris makes `NULL < NaN`, `NULL != NaN`, and `NULL NOT IN (NaN)`
UNKNOWN. In REWRITE mode this expression becomes the `TableScan` file filter
and residuals are ignored, so a null-only file is selected by a rewrite
filtered with `d < CAST('NaN' AS DOUBLE)` even though no row satisfies the
WHERE. The new equivalence test also omits null. Please preserve the non-null
domain for these negative NaN forms (including NOT-IN and negated forms) and
add nullable row/file-metrics coverage.
--
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]
