zeroshade commented on PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#issuecomment-5039317736
Thanks for your patience here, @C-Loftus — this is a real design fork and
it's on me to settle it. Short version: **let's pass the table properties
explicitly rather than through `context.Context`, and the PR needs a matching
read-side change before it can land.**
**ctx vs. explicit properties**
Widening `SchemaVisitorPerPrimitiveType` is still a no — but the answer
isn't context either, so I'm walking back the earlier steer in #1230 toward
reusing the ctx channel (this also lines up with the concern @laskoviymishka
raised in review). Two reasons:
1. The projjson document is a *required input* to the conversion, not a
request-scoped/cross-cutting value, so `context.Context` is the wrong channel —
correctness ends up riding on "did someone remember to call
`WithTableProperties`" instead of on the signature.
2. More concretely: the exported `SchemaToArrowSchema` / `TypeToArrowType`
hardcode `context.Background()`, so an external caller building a writer on the
public API can *never* resolve a `projjson:` CRS — they just get `"could not be
resolved from table properties"` with no way to pass them in. The read-path
`ctx` precedent is a reader-tuning hook (batch size, etc.), not
schema-conversion semantics, so I don't want to lean on it here.
What I'd like instead:
- Add exported `SchemaToArrowSchemaWithOptions(sc, opts)` and
`TypeToArrowTypeWithOptions(t, opts)`, where `opts` carries the existing flags
plus `TableProperties iceberg.Properties`. Keep the current
`SchemaToArrowSchema` / `TypeToArrowType` as thin wrappers that pass no
properties, so the existing public API and its callers are untouched.
- Put the properties (or a small CRS-resolver closure) directly on
`convertToArrow`. A short-lived visitor holding explicit conversion config is
fine and idiomatic — it's specifically storing `context.Context` on the struct
that I want to avoid.
- That also lets you drop the `...WithContext` internal helpers and the
`context.Background()` in `newWriterFactory` — that path currently drops the
caller's cancellation/allocator too, which is a latent footgun independent of
projjson.
- Internal write sites just set `opts.TableProperties = meta.props`.
**Read side (my blocking concern)**
The write half now emits `geoarrow.Metadata{CRS: <projjson doc>, CRSType:
PROJJSON}`, but `geoArrowCRSToIcebergCRS` takes the JSON-object branch and
ignores `CRSType` — it tries to pull `id.authority`/`id.code` out of the
document. So a table we write comes back either as a collapsed `authority:code`
(dropping the `projjson:<key>` reference), or, for an id-less PROJJSON doc, as
a hard `"unsupported CRS: missing id"`. Since
`GeometryType.Equals`/`GeographyType.Equals` compare the CRS string exactly,
that mismatch isn't cosmetic: our own scan/compaction/rewrite re-derives the
schema from the file and would no longer match `projjson:<key>`.
The property *name* isn't stored in the file, so exact recovery is only
possible with table context. What I'd want:
- On the table-aware read path, when `CRSType == PROJJSON`, recover the
original `projjson:<key>` by matching the field (by field ID) to the table
schema's geo field, validating that the referenced property's JSON matches the
embedded document, and reusing `projjson:<key>`.
- Where there's no table context (the bare `ArrowSchemaToIceberg`), return
an explicit error for `CRSType == PROJJSON` rather than silently degrading to
an authority code — we can't reconstruct the reference from file metadata alone.
**Merge bar**
I don't want to land write-only support, since it lets someone create a
table our own read/compaction path can't round-trip. Both halves should be in
before merge — but feel free to split them into two PRs (write-side resolution,
then read-side recovery) if that's easier to review. Happy to look at an
interface sketch before you build it out if you'd like a checkpoint.
--
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]
