gerinsp commented on PR #1179:
URL: https://github.com/apache/iceberg-go/pull/1179#issuecomment-4669299911
> The guard is right and clearly stops the panic, thanks for tracking it
down. A few concrete things before it merges:
>
> Now that `Partition()` returns nil instead of panicking, that nil flows
into eq-delete conflict validation. In `eqDeletePartitionsToFilter`
(conflict_validation.go:545) an identity-partitioned eq-delete calls
`anyToLiteral(p[partFieldID])`, and there's no nil guard before it —
`anyToLiteral(nil)` falls to the `default` at conflict_validation.go:603 and
returns `"unsupported partition value type <nil>"`, which fails the commit. So
for a null identity-partition value this trades a panic for a rejected commit.
Can you handle nil explicitly there (emit an `IS NULL` term) and add a test
that drives that path?
>
> On the test: it hand-builds the `dataFile` struct rather than decoding
avro, so it exercises the conversion's nil handling but not the avro decode
path that actually produced the nil — which is what the bug was. Could you
write and read back a real OCF with a nullable union partition field (`["null",
{"type":"int","logicalType":"date"}]`) so the test locks down the actual decode
behavior?
>
> Also, the `ValueCounts()` assertion goes through `initColumnStatsData()`,
not the partition path, so it passes with or without the fix —
`Partition()[1000]` is the only line doing real work in that test. Worth
dropping the `ValueCounts()` lines or reframing them as an explicit "stays
empty, doesn't panic" check.
Thanks that makes sense. I updated the patch to handle nil identity
partition values in `eqDeletePartitionsToFilter` by emitting an IS NULL
predicate instead of passing nil through `anyToLiteral`.
I also replaced the hand-build dataFile regression with a real manifest OCF
round-trip using a nullable logical date partition field, and removed the
`ValueCounts` assertion so the test focuses on the partition path.
Added coverage for the equality-delete conflict validation path with a null
identity partition as 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]
