badalprasadsingh commented on code in PR #2513:
URL: https://github.com/apache/iceberg-go/pull/2513#discussion_r4533479830
##########
table/evaluators.go:
##########
@@ -191,7 +191,8 @@ func (m *manifestEvalVisitor) visitIn(term
iceberg.BoundTerm, literals iceberg.S
hasExtrema := minLit != nil && maxLit != nil
if field.LowerBound == nil {
- return rowsCannotMatch
+ // An upper bound proves non-null partition values may exist.
+ return field.UpperBound != nil
}
Review Comment:
with this change the evaluator keeps one-bound summaries itself: so the same
check in `dynamicOverwriteManifestCandidates` (dynamic_partition_overwrite.go
linee138) becomes a duplicate
better: drop `(summary.LowerBound == nil) != (summary.UpperBound == nil) ||`
there, so the rule lives in one place
##########
table/evaluators.go:
##########
@@ -391,8 +392,8 @@ func (m *manifestEvalVisitor) VisitEqual(term
iceberg.BoundTerm, lit iceberg.Lit
field := m.partitionFields[pos]
if field.LowerBound == nil || field.UpperBound == nil {
- // values are all null and literal cannot contain null
- return rowsCannotMatch
+ // A single omitted bound cannot prove that all partition
values are null.
+ return field.LowerBound != nil || field.UpperBound != nil
}
Review Comment:
this one rule is spread over nine visitors, and results differ by filter:
with only lower bound `m`, `value IN ('a','b')` skips the manifest but `value =
'a'` keeps it
better: check once in `Eval`, like `dynamicOverwriteManifestCandidates`
does, and revert the visitor edits, would love to hear your thought on this.
##########
table/evaluators_partial_bounds_test.go:
##########
@@ -0,0 +1,107 @@
+// 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 (
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/require"
+)
+
+// Field-summary bounds are independently optional. A missing bound cannot
+// exclude matching partitions when the other bound is present.
+func TestManifestEvaluatorPartialPartitionBounds(t *testing.T) {
Review Comment:
nit: the spec does not make the bounds independent, it says a bound is "null
if all values are null or NaN".
better: "A summary with only one bound is invalid, so it cannot prove that
the manifest has no matches"
##########
table/evaluators_partial_bounds_test.go:
##########
@@ -0,0 +1,107 @@
+// 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 (
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/require"
+)
+
+// Field-summary bounds are independently optional. A missing bound cannot
+// exclude matching partitions when the other bound is present.
+func TestManifestEvaluatorPartialPartitionBounds(t *testing.T) {
+ schema := iceberg.NewSchema(1, iceberg.NestedField{
+ ID: 1, Name: "value", Type: iceberg.PrimitiveTypes.String,
+ })
+ spec := iceberg.NewPartitionSpec(iceberg.PartitionField{
+ SourceIDs: []int{1}, FieldID: 1000, Name: "value",
+ Transform: iceberg.IdentityTransform{},
+ })
+ lower := []byte("m")
+ upper := []byte("t")
+ ref := iceberg.Reference("value")
+
+ filters := []struct {
+ name string
+ expr iceberg.BooleanExpression
+ }{
+ {"less than", iceberg.LessThan(ref, "o")},
+ {"less than equal", iceberg.LessThanEqual(ref, "o")},
+ {"greater than", iceberg.GreaterThan(ref, "o")},
+ {"greater than equal", iceberg.GreaterThanEqual(ref, "o")},
+ {"equal", iceberg.EqualTo(ref, "o")},
+ {"in", iceberg.IsIn(ref, "o", "p")},
+ {"not null", iceberg.NotNull(ref)},
+ {"starts with", iceberg.StartsWith(ref, "o")},
+ }
+
+ for _, tc := range []struct {
+ name string
+ summary iceberg.FieldSummary
+ }{
+ {"lower bound only", iceberg.FieldSummary{ContainsNull: true,
LowerBound: &lower}},
+ {"upper bound only", iceberg.FieldSummary{ContainsNull: true,
UpperBound: &upper}},
+ } {
Review Comment:
please add:
1. with both bounds missing and ContainsNull: true, =, <, <=, >, >= and
startsWith still skip the manifest (only IN has such a case today)
2. a two-field spec with the one-bound summary on the second field, still
kept.
--
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]
