badalprasadsingh commented on code in PR #2507: URL: https://github.com/apache/iceberg-go/pull/2507#discussion_r4536057103
########## table/evaluators_manifest_exclusions_test.go: ########## @@ -0,0 +1,179 @@ +// 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" +) + +func TestManifestEvaluatorExcludedSingleValue(t *testing.T) { + nanFalse, nanTrue := false, true + ref := iceberg.Reference("value") + intSeven := iceberg.NewLiteral(int32(7)) + intEight := iceberg.NewLiteral(int32(8)) + floatSeven := iceberg.NewLiteral(float32(7)) + stringABC := iceberg.NewLiteral("abc") + binaryABC := iceberg.NewLiteral([]byte("abc")) + + tests := []struct { + name string + typ iceberg.Type + lower iceberg.Literal + upper iceberg.Literal + containsNull bool + containsNaN *bool + filter iceberg.BooleanExpression + want bool + }{ + { + name: "not equal excludes the only value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotEqualTo(ref, int32(7)), + }, + { + name: "not equal keeps a different value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotEqualTo(ref, int32(8)), want: true, + }, + { + name: "not in excludes the only value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotIn(ref, int32(5), int32(7)), + }, + { + name: "not in keeps a value outside the set", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotIn(ref, int32(5), int32(6)), want: true, + }, + { + name: "different bounds cannot prove one value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intEight, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "nulls may match not equal", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, containsNull: true, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "nulls may match not in", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, containsNull: true, + filter: iceberg.NotIn(ref, int32(5), int32(7)), want: true, + }, + { + name: "missing lower bound is unknown", + typ: iceberg.PrimitiveTypes.Int32, + upper: intSeven, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "missing upper bound is unknown", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, + filter: iceberg.NotIn(ref, int32(5), int32(7)), want: true, + }, + { + name: "missing both bounds is unknown", + typ: iceberg.PrimitiveTypes.Int32, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "unknown float NaN presence keeps manifest", + typ: iceberg.PrimitiveTypes.Float32, + lower: floatSeven, upper: floatSeven, + filter: iceberg.NotEqualTo(ref, float32(7)), want: true, + }, + { + name: "known float NaNs keep manifest", + typ: iceberg.PrimitiveTypes.Float32, + lower: floatSeven, upper: floatSeven, containsNaN: &nanTrue, + filter: iceberg.NotEqualTo(ref, float32(7)), want: true, + }, + { + name: "known absent NaNs allow not equal pruning", + typ: iceberg.PrimitiveTypes.Float32, + lower: floatSeven, upper: floatSeven, containsNaN: &nanFalse, + filter: iceberg.NotEqualTo(ref, float32(7)), + }, + { + name: "known absent NaNs allow not in pruning", + typ: iceberg.PrimitiveTypes.Float32, + lower: floatSeven, upper: floatSeven, containsNaN: &nanFalse, + filter: iceberg.NotIn(ref, float32(7), float32(8)), + }, + { + name: "string single value prunes", + typ: iceberg.PrimitiveTypes.String, + lower: stringABC, upper: stringABC, + filter: iceberg.NotEqualTo(ref, "abc"), + }, + { + name: "binary single value prunes", + typ: iceberg.PrimitiveTypes.Binary, + lower: binaryABC, upper: binaryABC, + filter: iceberg.NotIn(ref, []byte("abc"), []byte("def")), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + schema := iceberg.NewSchema(1, iceberg.NestedField{ + ID: 1, Name: "value", Type: tt.typ, Required: false, + }) + spec := iceberg.NewPartitionSpec(iceberg.PartitionField{ + SourceIDs: []int{1}, + FieldID: 1000, + Name: "value", + Transform: iceberg.IdentityTransform{}, + }) Review Comment: every case uses a one-field spec, so reading the wrong field's summary would go unnoticed better: add these cases to TestManifestEvaluator, as apache/iceberg#15064 did in iceberg-ava: its spec has 13 fields and already has "no_nulls_same_value_a" and "all_same_value_or_null" ########## table/evaluators.go: ########## @@ -232,9 +232,42 @@ func (m *manifestEvalVisitor) visitIn(term iceberg.BoundTerm, literals iceberg.S return rowsMightMatch } +// uniqueValue returns a partition value only when the manifest summary proves +// that every partition has that value. Unknown NaN information is not sufficient +// for floating-point fields because NaNs are excluded from the bounds. +func (m *manifestEvalVisitor) uniqueValue(term iceberg.BoundTerm) iceberg.Literal { + field := m.partitionFields[term.Ref().Pos()] + if field.ContainsNull || field.LowerBound == nil || field.UpperBound == nil { + return nil + } + + switch term.Ref().Type().(type) { + case iceberg.Float32Type, iceberg.Float64Type: + if field.ContainsNaN == nil || *field.ContainsNaN { + return nil + } + } + + lower, err := iceberg.LiteralFromBytes(term.Ref().Type(), *field.LowerBound) + if err != nil { + return nil + } + upper, err := iceberg.LiteralFromBytes(term.Ref().Type(), *field.UpperBound) + if err != nil { + return nil + } Review Comment: nit: ither visitors panic(err) on a bound they can't decode; this returns nil and keeps the manifest, which is the safer choice for a pure optimization a short comment saying so would stop a later cleanup from switching it to panic ########## table/evaluators.go: ########## @@ -232,9 +232,42 @@ func (m *manifestEvalVisitor) visitIn(term iceberg.BoundTerm, literals iceberg.S return rowsMightMatch } +// uniqueValue returns a partition value only when the manifest summary proves +// that every partition has that value. Unknown NaN information is not sufficient +// for floating-point fields because NaNs are excluded from the bounds. +func (m *manifestEvalVisitor) uniqueValue(term iceberg.BoundTerm) iceberg.Literal { + field := m.partitionFields[term.Ref().Pos()] + if field.ContainsNull || field.LowerBound == nil || field.UpperBound == nil { + return nil + } + + switch term.Ref().Type().(type) { + case iceberg.Float32Type, iceberg.Float64Type: + if field.ContainsNaN == nil || *field.ContainsNaN { + return nil + } + } + + lower, err := iceberg.LiteralFromBytes(term.Ref().Type(), *field.LowerBound) + if err != nil { + return nil + } + upper, err := iceberg.LiteralFromBytes(term.Ref().Type(), *field.UpperBound) + if err != nil { + return nil + } + if getCmpLiteral(lower)(lower, upper) != 0 { + return nil + } + + return lower +} + func (m *manifestEvalVisitor) VisitNotIn(term iceberg.BoundTerm, literals iceberg.Set[iceberg.Literal]) bool { - // because the bounds are not necessarily a min or max value, this cannot be answered using them - // notIn(col, {X, ...}) with (X, Y) doesn't guarantee that X is a value in col + if value := m.uniqueValue(term); value != nil && literals.Contains(value) { + return rowsCannotMatch + } Review Comment: P1: the literal set treats -0.0 and 0.0 as equal, but the row filter (Arrow is_in) does not, and iceberg-ava keeps this manifest, so on a table partitioned by d, `Delete(d NOT IN (0.0, 9.0))` no longer removes -0.0 rows (Overwrite too) better: skip float zeros, plus a test ########## table/evaluators_manifest_exclusions_test.go: ########## @@ -0,0 +1,179 @@ +// 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" +) + +func TestManifestEvaluatorExcludedSingleValue(t *testing.T) { + nanFalse, nanTrue := false, true + ref := iceberg.Reference("value") + intSeven := iceberg.NewLiteral(int32(7)) + intEight := iceberg.NewLiteral(int32(8)) + floatSeven := iceberg.NewLiteral(float32(7)) + stringABC := iceberg.NewLiteral("abc") + binaryABC := iceberg.NewLiteral([]byte("abc")) + + tests := []struct { + name string + typ iceberg.Type + lower iceberg.Literal + upper iceberg.Literal + containsNull bool + containsNaN *bool + filter iceberg.BooleanExpression + want bool + }{ + { + name: "not equal excludes the only value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotEqualTo(ref, int32(7)), + }, + { + name: "not equal keeps a different value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotEqualTo(ref, int32(8)), want: true, + }, + { + name: "not in excludes the only value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotIn(ref, int32(5), int32(7)), + }, + { + name: "not in keeps a value outside the set", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, + filter: iceberg.NotIn(ref, int32(5), int32(6)), want: true, + }, + { + name: "different bounds cannot prove one value", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intEight, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "nulls may match not equal", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, containsNull: true, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "nulls may match not in", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, upper: intSeven, containsNull: true, + filter: iceberg.NotIn(ref, int32(5), int32(7)), want: true, + }, + { + name: "missing lower bound is unknown", + typ: iceberg.PrimitiveTypes.Int32, + upper: intSeven, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "missing upper bound is unknown", + typ: iceberg.PrimitiveTypes.Int32, + lower: intSeven, + filter: iceberg.NotIn(ref, int32(5), int32(7)), want: true, + }, + { + name: "missing both bounds is unknown", + typ: iceberg.PrimitiveTypes.Int32, + filter: iceberg.NotEqualTo(ref, int32(7)), want: true, + }, + { + name: "unknown float NaN presence keeps manifest", + typ: iceberg.PrimitiveTypes.Float32, + lower: floatSeven, upper: floatSeven, + filter: iceberg.NotEqualTo(ref, float32(7)), want: true, + }, + { + name: "known float NaNs keep manifest", + typ: iceberg.PrimitiveTypes.Float32, + lower: floatSeven, upper: floatSeven, containsNaN: &nanTrue, + filter: iceberg.NotEqualTo(ref, float32(7)), want: true, + }, Review Comment: no test reaches the "Float64Type" part of the NaN check, a wrong prune there drops NaN rows from != scans better: add double cases: unknown and known NaN keep the manifest -- 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]
