zeroshade commented on code in PR #1671:
URL: https://github.com/apache/iceberg-go/pull/1671#discussion_r3732159015
##########
table/conflict_validation.go:
##########
@@ -394,6 +392,10 @@ func validateAddedDataFilesMatchingFilter(ctx
*conflictContext, filter iceberg.B
partitionEvals := newKeyDefaultMapWrapErr(func(specID int)
(func(iceberg.DataFile) (bool, error), error) {
return buildPartitionEvaluator(specID, ctx.current,
ctx.current.CurrentSchema(), partitionFilters, ctx.caseSensitive)
})
+ metricsEval, err :=
newInclusiveMetricsEvaluator(ctx.current.CurrentSchema(), filter,
ctx.caseSensitive, false)
+ if err != nil {
+ return fmt.Errorf("failed to build metrics evaluator: %w", err)
+ }
for _, snap := range ctx.concurrent {
Review Comment:
Worth a one-line comment noting that hoisting `metricsEval` out of this loop
is safe because `Eval` copies its per-file state into a fresh struct on each
call (`table/evaluators.go:807-811`). The hoist is correct, but the
justification lives in another file.
##########
table/conflict_validation.go:
##########
@@ -394,6 +392,10 @@ func validateAddedDataFilesMatchingFilter(ctx
*conflictContext, filter iceberg.B
partitionEvals := newKeyDefaultMapWrapErr(func(specID int)
(func(iceberg.DataFile) (bool, error), error) {
return buildPartitionEvaluator(specID, ctx.current,
ctx.current.CurrentSchema(), partitionFilters, ctx.caseSensitive)
})
+ metricsEval, err :=
newInclusiveMetricsEvaluator(ctx.current.CurrentSchema(), filter,
ctx.caseSensitive, false)
Review Comment:
Non-blocking: the trailing `false` is `includeEmptyFiles`, which means a
concurrently added data file with `record_count == 0` hits the
`rowsCannotMatch` early return (`table/evaluators.go:803-804`) and is silently
skipped. That is defensible — there are no rows to lose — but it is currently
neither documented nor tested, so the next reader has to derive it. Consider a
short comment justifying the choice, or passing `true` if you'd rather keep
this check maximally conservative.
##########
table/conflict_validation_test.go:
##########
@@ -734,6 +734,73 @@ func
TestValidateAddedDataFilesMatchingFilter_NoConcurrent(t *testing.T) {
require.NoError(t, validateAddedDataFilesMatchingFilter(ctx, nil))
}
+func TestValidateAddedDataFilesMatchingFilterUsesFileMetrics(t *testing.T) {
+ tests := []struct {
+ name string
+ lower, upper int64
+ wantConflict bool
+ }{
+ {name: "bounds cannot match", lower: 100, upper: 200,
wantConflict: false},
Review Comment:
The case I'd most like to see added here: a data file with **no** bounds at
all, which must still report a conflict. That is the entire safety argument for
this change — absent metrics fall through to `rowsMightMatch` — and it's the
one property these three cases don't exercise. Non-blocking, but it's the
highest-value addition to this table.
##########
table/conflict_validation.go:
##########
@@ -358,18 +358,16 @@ func validateDataFilesExist(ctx *conflictContext,
referencedPaths []string) erro
// predicate call this so that a concurrent append into the same
// partition is rejected before the commit overwrites it.
//
-// The check runs in two layers:
+// The check runs in three layers:
// 1. A manifest-level partition-summary evaluator prunes manifests
// whose summaries cannot overlap the filter.
// 2. Inside surviving manifests, every ADDED entry attributed to the
// concurrent snapshot is evaluated against a per-spec partition
// evaluator on its partition tuple, so a manifest whose summary
// straddles the filter only triggers a conflict when at least
// one actual added file's partition value satisfies the filter.
-//
-// Per-file metric evaluation (a third pass in Java that refines
-// beyond partition for columns not in the spec) is TODO and tracked
-// under issue #830 follow-ups.
+// 3. The surviving files are evaluated against their column metrics so
+// unpartitioned files whose bounds cannot match the filter are ignored.
Review Comment:
Small wording nit. This bullet — and the PR description — says
"unpartitioned files", but the code applies `metricsEval` to every file that
survives the partition check, under every spec. The behavior is the right one;
the wording just understates its scope. Consider dropping "unpartitioned" so
the doc matches what the loop actually does.
##########
table/conflict_validation_test.go:
##########
@@ -734,6 +734,73 @@ func
TestValidateAddedDataFilesMatchingFilter_NoConcurrent(t *testing.T) {
require.NoError(t, validateAddedDataFilesMatchingFilter(ctx, nil))
}
+func TestValidateAddedDataFilesMatchingFilterUsesFileMetrics(t *testing.T) {
Review Comment:
Further coverage worth picking up over time, roughly in priority order after
the no-bounds case: `record_count == 0`; a file carrying bounds for other
columns but not the filter column; NaN or float bounds; a null-count-only
column paired with an `IsNull` filter; a truncated string upper bound; and a
genuinely partitioned spec with a filter on a non-partition column — the helper
below writes against `UnpartitionedSpec`, so the partitioned scenario the PR
description highlights isn't reached yet. All suggestions; the three cases here
do cover the core int64 path well.
--
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]
