github-actions[bot] commented on code in PR #68128:
URL: https://github.com/apache/doris/pull/68128#discussion_r4055857151
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPredicateConverter.java:
##########
@@ -772,13 +919,17 @@ private Expression
buildConflictComparison(ConnectorComparison cmp) {
if (value == null) {
return null;
}
+ if (isNaNValue(value)) {
+ return isFloatingPoint(type) ?
buildNaNComparison(cmp.getOperator(), colName) : null;
+ }
switch (cmp.getOperator()) {
case EQ:
return Expressions.equal(colName, value);
case GT:
- return Expressions.greaterThan(colName, value);
+ // Missing a NaN-holding file here means missing a real write
conflict; see withNaN.
+ return withNaN(field, Expressions.greaterThan(colName, value));
Review Comment:
[P1] Handle NaN in the identity-partition conflict filter
This fixes the query-filter arm, but `applyConflictDetectionFilter` also
ANDs it with `buildConflictDetectionFilter`. For an identity-partitioned
FLOAT/DOUBLE table, a commit fragment carrying `nan` is deliberately parsed to
`Float.NaN`/`Double.NaN`, and `buildIdentityPartitionExpression` still sends
that value to `Expressions.equal`, which Iceberg rejects immediately. Thus
DELETE/UPDATE/MERGE touching a valid NaN identity partition still fails before
conflict validation. Please map that partition value to `isNaN` (or
conservatively disable partition narrowing) and cover the transaction path with
a NaN identity-partition test.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPredicateConverter.java:
##########
@@ -287,9 +313,118 @@ private Expression buildIn(ConnectorIn in) {
// A single unconvertible element drops the whole IN/NOT-IN
(legacy parity).
return null;
}
+ if (isNaNValue(value)) {
+ // Expressions.in would throw on a NaN literal. Doris compares
NaN = NaN as true, so the element
+ // matches exactly the NaN rows -- iceberg spells that isNaN /
notNaN.
+ if (!isFloatingPoint(field.type())) {
+ return null;
+ }
+ hasNaN = true;
+ continue;
+ }
values.add(value);
}
- return in.isNegated() ? Expressions.notIn(colName, values) :
Expressions.in(colName, values);
+ if (!hasNaN) {
+ return in.isNegated() ? Expressions.notIn(colName, values) :
Expressions.in(colName, values);
+ }
+ Expression nan = in.isNegated() ? Expressions.notNaN(colName) :
Expressions.isNaN(colName);
+ if (values.isEmpty()) {
+ return nan;
+ }
+ return in.isNegated()
+ ? Expressions.and(Expressions.notIn(colName, values), nan)
+ : Expressions.or(Expressions.in(colName, values), nan);
+ }
+
+ private static boolean isFloatingPoint(Type type) {
+ return type.typeId() == TypeID.FLOAT || type.typeId() == TypeID.DOUBLE;
+ }
+
+ private static boolean isNaNValue(Object value) {
+ return (value instanceof Double && ((Double) value).isNaN())
+ || (value instanceof Float && ((Float) value).isNaN());
+ }
+
+ /**
+ * Doris orders NaN above every other floating-point value
(be/src/common/compare.h GreaterThanFloat), so a
+ * NaN row satisfies {@code col > v} and {@code col >= v}. Iceberg's
metrics/manifest evaluators assume the
+ * opposite -- NaN is excluded from the lower/upper bounds and an all-NaN
file is pruned for any range
+ * predicate -- which silently drops those rows before BE ever sees the
split (DORIS-29047). OR in isNaN so
+ * a file that may hold a NaN survives; a file whose nan_value_count is 0
is still pruned by the range arm.
+ * LT/LE need no such arm: Doris evaluates {@code NaN < v} / {@code NaN <=
v} as false, matching iceberg.
+ */
+ private static Expression withNaN(Types.NestedField field, Expression
range) {
+ return isFloatingPoint(field.type())
+ ? Expressions.or(range, Expressions.isNaN(field.name()))
+ : range;
+ }
+
+ /** The Doris NaN-literal comparisons iceberg can express exactly; the
rest are left to BE. */
+ private static Expression buildNaNComparison(ConnectorComparison.Operator
op, String colName) {
+ switch (op) {
+ case EQ:
+ case EQ_FOR_NULL:
+ case GE:
+ // NaN = NaN and NaN >= NaN hold in Doris, and nothing else
reaches NaN from below.
+ return Expressions.isNaN(colName);
+ case NE:
+ case LT:
+ return Expressions.notNaN(colName);
+ default:
+ // col > NaN is never true and col <= NaN is always true for a
non-null row: no narrowing form.
+ return null;
+ }
+ }
+
+ /** Doris negation of a range comparison; exact because non-null floats
are totally ordered there. */
+ private static ConnectorComparison.Operator
negateRange(ConnectorComparison.Operator op) {
+ switch (op) {
+ case LT:
+ return ConnectorComparison.Operator.GE;
+ case LE:
+ return ConnectorComparison.Operator.GT;
+ case GT:
+ return ConnectorComparison.Operator.LE;
+ case GE:
+ return ConnectorComparison.Operator.LT;
+ default:
+ return null;
+ }
+ }
+
+ private boolean isFloatingPointColumn(ConnectorExpression expr) {
+ if (!(expr instanceof ConnectorColumnRef)) {
+ return false;
+ }
+ Types.NestedField field = getPushdownField(((ConnectorColumnRef)
expr).getColumnName());
+ return field != null && isFloatingPoint(field.type());
+ }
+
+ /**
+ * Whether negating {@code expr} would put a float/double range predicate
into iceberg's IEEE-flavoured
+ * negation, i.e. whether the subtree holds a {@code col < v} / {@code col
<= v} / {@code col BETWEEN lo AND
+ * hi} on a floating-point column. Those are exactly the forms whose
negation must carry an isNaN arm and
+ * cannot get one from {@code Expressions.not}. GT/GE are not listed: this
converter already emits them as
+ * {@code (range or isNaN)}, which iceberg negates correctly into {@code
(ltEq and notNaN)}.
+ */
+ private boolean containsNegatableFloatRange(ConnectorExpression expr) {
+ if (expr instanceof ConnectorComparison) {
+ ConnectorComparison cmp = (ConnectorComparison) expr;
+ ConnectorComparison.Operator op = cmp.getOperator();
+ if ((op == ConnectorComparison.Operator.LT || op ==
ConnectorComparison.Operator.LE)
+ && isFloatingPointColumn(cmp.getLeft())) {
+ return true;
+ }
+ } else if (expr instanceof ConnectorBetween
+ && isFloatingPointColumn(((ConnectorBetween)
expr).getValue())) {
+ return true;
+ }
+ for (ConnectorExpression child : expr.getChildren()) {
+ if (containsNegatableFloatRange(child)) {
Review Comment:
[P1] Track negation parity inside compound NOT
The recursive check looks only at raw operators, so `NOT (NOT (d > 5) AND i
= 1)` passes: the inner source operator is GT and is not flagged. Conversion
flips that inner NOT to bare `d <= 5`, then Iceberg rewrites the outer NOT to
`d > 5 OR i != 1`, with no `isNaN` arm. A file containing `(NaN,1)` and `(1,1)`
is therefore pruned from `rewrite_data_files` even though Doris makes the
original predicate true for the NaN row. This procedure path preserves nested
NOTs, so please make the analysis negation-context aware (or normalize under
Doris ordering) and cover this producer shape.
--
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]
