laskoviymishka commented on code in PR #1191:
URL: https://github.com/apache/iceberg-go/pull/1191#discussion_r3412846208
##########
table/evaluators_test.go:
##########
@@ -715,6 +715,53 @@ func TestManifestEvaluator(t *testing.T) {
})
}
+func TestManifestEvaluatorWithDroppedPartitionSource(t *testing.T) {
Review Comment:
This covers a dropped field at slot 0, but the more interesting case is a
drop in the *middle* of a 3-field spec, which shifts two subsequent fields
rather than one.
I'd add a `[a_part(src=dropped), b_part(src=present), c_part(src=dropped)]`
case with the predicate on `b_part`, asserting it's evaluated at slot 1. That's
where an off-by-one in the placeholder logic would actually surface — the
current single-drop-at-slot-0 case wouldn't catch it.
##########
table/evaluators_test.go:
##########
@@ -715,6 +715,53 @@ func TestManifestEvaluator(t *testing.T) {
})
}
+func TestManifestEvaluatorWithDroppedPartitionSource(t *testing.T) {
+ qMin, err := iceberg.Int64Literal(10).MarshalBinary()
+ require.NoError(t, err)
+ qMax, err := iceberg.Int64Literal(20).MarshalBinary()
+ require.NoError(t, err)
+ pMin, err := iceberg.Int64Literal(2).MarshalBinary()
+ require.NoError(t, err)
+ pMax, err := iceberg.Int64Literal(2).MarshalBinary()
+ require.NoError(t, err)
+
+ spec := iceberg.NewPartitionSpec(
+ iceberg.PartitionField{SourceIDs: []int{3}, FieldID: 1000,
Name: "q_part", Transform: iceberg.IdentityTransform{}},
+ iceberg.PartitionField{SourceIDs: []int{2}, FieldID: 1001,
Name: "p_part", Transform: iceberg.IdentityTransform{}},
+ )
+
+ currentSchema := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ iceberg.NestedField{ID: 2, Name: "p", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ )
+ manifest := iceberg.NewManifestFile(1, "", 0, 0, 0).Partitions(
+ []iceberg.FieldSummary{
+ {
+ ContainsNull: false,
+ LowerBound: &qMin,
+ UpperBound: &qMax,
+ },
+ {
+ ContainsNull: false,
+ LowerBound: &pMin,
+ UpperBound: &pMax,
+ },
+ }).Build()
+
+ project := newInclusiveProjection(currentSchema, spec, true)
+ partitionFilter, err := project(iceberg.EqualTo(iceberg.Reference("p"),
int64(2)))
+ require.NoError(t, err)
+
+ // Manifest partition summaries are positional. The manifest was written
+ // with q_part at slot 0 and p_part at slot 1; dropping q from the
current
+ // schema must not compact p_part to slot 0 during manifest evaluation.
+ eval, err := newManifestEvaluator(spec, currentSchema, partitionFilter,
true)
+ require.NoError(t, err)
+ result, err := eval(manifest)
+ require.NoError(t, err)
+ assert.True(t, result, "p_part must be evaluated against slot 1, not
q_part's slot 0")
Review Comment:
The only assertion here is `assert.True`, which a broken fix that returns
`rowsMightMatch` unconditionally would also pass — so it doesn't actually prove
the evaluator is reading slot 1.
Could we add a negative case? A second manifest where `p_part`'s bounds
don't contain 2 (say 3..5), asserting `eval` returns `false`. That pins both
that the right slot is read *and* that pruning still works after the fix.
--
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]
