laskoviymishka commented on issue #1230: URL: https://github.com/apache/iceberg-go/issues/1230#issuecomment-4915587211
My preference: don't change the SchemaToArrowSchema/TypeToArrowType signatures, and don't put properties on the visitor either. We already have a channel for exactly this. The read path threads table properties through context (arrow_scanner.go does ctx = internal.WithTableProperties(ctx, as.metadata.Properties()) and parquet_files.go reads it back on the other side). This is the write-side mirror of that same problem, so I'd rather reuse that pattern than invent a second mechanism. Concretely: resolve the projjson:<name> reference at the boundary via a ctx-aware entry point that pulls props from context and materializes the CRS before we visit. The visitor then runs on already-resolved values, so convertToArrow and VisitGeometry/VisitGeography stay unchanged, icebergCRSToGeoArrowMetadata never learns what a table property is, and the interface plus the other implementers (convertToSubstrait, test visitors) are untouched. The write-path callers all have Properties() on hand, so populating the context is cheap at each site. Carrying props on the visitor struct (Tanmay's option) works too and keeps it local, but it still needs the props fed in through the exported entry points, and it leaks the properties concept into the visitor for what's a pretty narrow feature. The context route avoids both. And agreed on the second point: resolve-only at write time, no injecting or defaulting PROJJSON on table creation. That's the caller's job per the spec. -- 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]
