Revanth14 commented on PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#issuecomment-4794334302
> Nice addition — this locks down the exact `geometry(...)`/`geography(...)`
type-string literals that Java's `SchemaParser.toJson` emits, which is exactly
the kind of thing that silently drifts when someone tweaks `String()`
formatting. The four parameterized cases you picked are right.
>
> Two things make the fixture hard to trust as a reference, though, and
since the whole point is to be the thing others build on, I'd fix them before
merge. The cited Java commit `df6ea082...` isn't the geo-serialization commit —
it's a metrics-reporter PR (#16664), so anyone tracing the provenance hits a
dead end. And the comment frames this as a cross-implementation format, but
PyIceberg emits _quoted_ CRS (`geometry('srid:3857')`) and won't parse the
unquoted Java/Go form — so "cross-implementation" overstates what's pinned here.
>
> On the test itself: the top-level `require.Equal` is mostly carried by the
four literal asserts below it, and the most common wire form — bare
`geometry`/`geography` with default CRS — isn't covered. Both worth tightening.
>
> Fix the commit citation and the cross-impl note, close the default-case
gap, and this is good to land.
Thanks for the detailed pass. I pushed the follow-up with the provenance
fixed to the geo-related Iceberg commits, added the PyIceberg quoted-CRS note
so the fixture is clearly scoped to Java's form, replaced the broad map
equality with explicit per-field byte-string assertions plus length checks, and
added bare `geometry` / `geography` default cases to the Java fixture.
--
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]
