zeroshade commented on code in PR #2380:
URL: https://github.com/apache/iceberg-go/pull/2380#discussion_r4469820269
##########
table/conflict_validation.go:
##########
@@ -923,37 +979,23 @@ func eqDeletePartitionsToFilter(files []iceberg.DataFile,
meta Metadata) (iceber
specInfoByID[specID] = specInfo
}
- // Sort partition field IDs for deterministic expression order.
fieldIDs := make([]int, 0, len(p))
Review Comment:
This PR removes several comments that still apply: "Sort partition field IDs
for deterministic expression order", the note that `canonicalPartitionKey` is
injective, and "Resolve each source field at most once per partition spec…"
(which backs the `currentSchemaCalls == 1` check). Could these be restored?
##########
table/conflict_validation.go:
##########
@@ -876,6 +877,59 @@ type equalityDeleteSourceFieldInfo struct {
found bool
}
+// NaNs are not equal to themselves as Go map keys, while the existing
+// equality evaluator compares two NaNs as equal. Binary sets use hash keys,
+// which can collide, so retain individual predicates for those values too.
+func equalityDeleteLiteralCanUseSet(lit iceberg.Literal) bool {
+ switch value := lit.(type) {
+ case iceberg.Float32Literal:
+ return !math.IsNaN(float64(value))
+ case iceberg.Float64Literal:
+ return !math.IsNaN(float64(value))
+ case iceberg.BoolLiteral, iceberg.Int32Literal, iceberg.Int64Literal,
+ iceberg.DateLiteral, iceberg.TimeLiteral,
iceberg.TimestampLiteral,
+ iceberg.TimestampNsLiteral, iceberg.StringLiteral,
iceberg.UUIDLiteral, iceberg.DecimalLiteral:
+ return true
+ default:
+ return false
+ }
+}
+
+type equalityDeleteIdentityGroupKey struct {
+ specID int
+ fieldID int
+}
+
+type equalityDeleteIdentityGroup struct {
+ fieldName string
+ fieldType iceberg.Type
+ index int
+ literals []iceberg.Literal
+}
+
+// balancedConflictDisjunction keeps multi-field partition filters shallow.
+// The old left fold required traversal depth proportional to the number of
Review Comment:
Please describe current behaviour rather than "the old left fold".
##########
table/conflict_validation.go:
##########
@@ -923,37 +979,23 @@ func eqDeletePartitionsToFilter(files []iceberg.DataFile,
meta Metadata) (iceber
specInfoByID[specID] = specInfo
}
- // Sort partition field IDs for deterministic expression order.
fieldIDs := make([]int, 0, len(p))
for id := range p {
fieldIDs = append(fieldIDs, id)
}
sort.Ints(fieldIDs)
- // Non-identity transforms (bucket, day, hour, truncate, year,
month) store
- // post-transform values in DataFile.Partition(). Building a
row-space predicate
- // with those values would cause
validateAddedDataFilesMatchingFilter to
- // re-apply the transform, producing wrong matches
(double-transformation).
- // Fall back to AlwaysTrue (conservative: treat the eq-delete
as table-wide)
- // until a full PartitionSet-style partition-space approach is
added.
- identityOnly := true
+ // A non-identity value is already transformed, so applying
another
Review Comment:
The previous comment explained why: identity-transform values are
re-transformed in row space (double transformation), and the fallback stays
until a PartitionSet-style approach exists. Could that detail be kept? The
`identityOnly` → early-return rewrite also seems unrelated to this change.
##########
table/eq_delete_predicate_scale_bench_test.go:
##########
@@ -0,0 +1,77 @@
+// 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 table
+
+import (
+ "fmt"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+)
+
+var eqDeleteConflictPredicateScaleSink iceberg.BooleanExpression
+
+// BenchmarkEqDeleteConflictPredicateScale measures the end-to-end predicate
+// construction and schema binding used by serializable equality-delete
+// conflict checks. Run the same benchmark on the upstream base and this
Review Comment:
"Run the same benchmark on the upstream base and this branch" is about this
PR. Also, `BenchmarkEqDeletePartitionsToFilter` in
`partition_conflict_bench_test.go` already benchmarks this function with the
same input helper; could it get a bind step instead of a new file?
##########
table/conflict_validation.go:
##########
@@ -995,25 +1036,62 @@ func eqDeletePartitionsToFilter(files
[]iceberg.DataFile, meta Metadata) (iceber
return nil, fmt.Errorf("partition field %q:
%w", sourceField.name, err)
}
- conjuncts = append(conjuncts,
iceberg.LiteralPredicate(iceberg.OpEQ, iceberg.Reference(sourceField.name),
lit))
+ if len(fieldIDs) == 1 && len(files) >= 32 &&
equalityDeleteLiteralCanUseSet(lit) {
Review Comment:
What is `32` based on? It counts input files rather than distinct
partitions, and grouping at 16 partitions costs about the same. If it only
keeps the exact expressions expected by existing small tests, consider removing
it and updating those tests; otherwise make it a named constant with a reason.
Also, this block looks up the field again with `FindFieldByID` (ignoring the
found flag) right after `specInfo.sourceFields` resolved it. Storing the type
in `equalityDeleteSourceFieldInfo` would avoid that.
##########
table/eq_delete_conflict_perf_test.go:
##########
@@ -0,0 +1,168 @@
+// 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 table
+
+import (
+ "fmt"
+ "math"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/require"
+)
+
+func TestEqDeleteConflictSingleFieldWithNullAndSeparatedValues(t *testing.T) {
Review Comment:
Could these tests go in `partition_conflict_test.go` with the other
`eqDeletePartitionsToFilter` tests? `TestEqDeleteConflictSetLiteralSafety`
calls the helper directly (the NaN test already covers it through the real
path), and `TestBalancedConflictDisjunctionPreservesTerms` checks only the tree
shape. Consider dropping both.
--
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]
