hubgeter commented on code in PR #68128:
URL: https://github.com/apache/doris/pull/68128#discussion_r4068048734
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java:
##########
@@ -922,6 +918,28 @@ private Expression buildPartitionFilter(Map<String,
String> staticPartitions, Pa
return result;
}
+ /**
+ * {@code sourceCol = value} for one identity partition key, or the unary
predicate the value demands.
+ *
+ * <p>A NaN cannot be an iceberg literal at all — {@code Literals.from}
throws "Cannot create expression
+ * literal from NaN", and iceberg models it only through {@code
isNaN}/{@code notNaN}. Meanwhile
+ * {@link IcebergPartitionUtils#parsePartitionValueFromString}
deliberately parses Doris's {@code nan}
+ * spelling into {@link Double#NaN}, so a FLOAT/DOUBLE identity partition
holding NaN used to abort the
+ * whole commit ("Failed to commit iceberg transaction: Cannot create
expression literal from NaN") on
+ * both paths that build this predicate: DELETE/UPDATE/MERGE conflict
detection and
+ * {@code INSERT OVERWRITE ... PARTITION(d='nan')}.
+ */
+ private static Expression identityPartitionPredicate(String sourceColName,
Object value) {
+ if (value == null) {
+ return Expressions.isNull(sourceColName);
+ }
+ if ((value instanceof Double || value instanceof Float)
+ && Double.isNaN(((Number) value).doubleValue())) {
+ return Expressions.isNaN(sourceColName);
Review Comment:
Thanks — the test-coverage half is fair and I've addressed it. The narrowing
itself I don't think belongs to this PR, and I checked rather than argued.
**The missed conflict is not NaN- or signed-zero-specific, and not new
here.** I ran the same scenario with a plain `INT` identity partition, no
floating point anywhere:
```
INT: DELETE WHERE r > 5, base partition r=7, concurrent append r=10
-> commit SUCCEEDED, conflict missed
DOUBLE: DELETE WHERE d > 5, base partition d=NaN, concurrent append d=10.0
-> commit SUCCEEDED, conflict missed
```
`buildConflictDetectionFilter` builds its OR only over the partitions that
appear in the commit fragments, so *any* partition the predicate matches but
that had no matching rows at scan time falls outside the filter. That is
pre-existing behaviour of identity-partition narrowing on master and is
orthogonal to how a NaN partition value is spelled; this PR's diff to
`IcebergConnectorTransaction` is only the `Expressions.equal(col, NaN)` ->
`isNaN(col)` mapping, which is what previously made the commit throw `Cannot
create expression literal from NaN` before validation ran at all.
Using the query filter alone when present would also remove the narrowing
that `deletePartitionedIdentityNarrowsConflictDetectionToTouchedPartition`
explicitly asserts (a concurrent append to a *different* partition must not
conflict), so it is a behaviour change for every partitioned DELETE rather than
a fix to this one. Happy to file it separately if you'd like it tracked.
**What I did take from the comment:** the new test really did only prove an
exception-free commit, with neither `applyWriteConstraint` nor a concurrent
append. Two cases added:
- `deleteOnNaNIdentityPartitionStillDetectsConcurrentConflict` — `DELETE ...
WHERE d > 5` with `applyWriteConstraint`, racing an append into the **same**
NaN partition; requires the commit to fail *and* the conflicting-files message
to contain `is_nan`. That last assertion is the load-bearing one: it separates
"failed because of a real conflict" from "failed on the NaN literal", so a
dropped NaN arm is caught. The filter it exercises is `((d > 5 or is_nan(d))
and is_nan(d))`.
- `deleteOnNaNIdentityPartitionExcludesNonMatchingPartition` — racing an
append into `d=1.0`, which `d > 5` does not match; requires the commit to
succeed.
Together they pin that the NaN predicate narrows, rather than degrading to
always-true or always-false. Both fail on unpatched master.
--
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]
