You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "C-Loftus (via GitHub)" <gi...@apache.org> on 2026/07/08 16:00:21 UTC
[PR] feat(table): projjson write level support [iceberg-go]
C-Loftus opened a new pull request, #1426:
URL: https://github.com/apache/iceberg-go/pull/1426
Closes #1230
This PR threads through projjson definition metadata by using a `ctx` variable in accordance to the guidance in #1230 . Note that some of the writer call stack did not previously take a `ctx` variable. As such, anything public has an additional new private helper that takes in a `ctx` to resolve it.
I modified `icebergCRSToGeoArrowMetadata` to take in a `ctx` as well so it can resolve either normal inline crs or projjson.
This ultimately ends up giving us a VisitGeometry API like the following
```go
func (c convertToArrow) VisitGeometry(g iceberg.GeometryType) arrow.Field {
meta, err := icebergCRSToGeoArrowMetadata(c.ctx, g.CRS())
/// rest removed for brevity ....
}
```
Happy to refactor as maintainers see fit. I wasn't entirely sure on a few of the refactors and whether or not there is a better way to thread the context through / if it is reasonable to change the public methods for things like `SchemaToArrowSchema`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson write level support [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#discussion_r3566791334
##########
table/rolling_data_writer.go:
##########
@@ -108,6 +108,7 @@ func withFactoryFileSchema(schema *iceberg.Schema) writerFactoryOption {
// configuration derived from the table metadata.
func newWriterFactory(rootLocation string, args recordWritingArgs, meta *MetadataBuilder, taskSchema *iceberg.Schema, targetFileSize int64, opts ...writerFactoryOption) (*writerFactory, error) {
nextCount, stopCount := iter.Pull(args.counter)
+ propCtx := tblutils.WithTableProperties(context.Background(), meta.props)
Review Comment:
Minor: this builds the prop-context from `context.Background()` rather than a caller-supplied ctx, so a caller's cancellation/deadline won't propagate into the schema conversion done here. If a request ctx is reachable at this call site, threading it through would be more consistent with the other write paths in this PR.
##########
table/arrow_utils.go:
##########
@@ -528,6 +528,10 @@ func ensureSmallArrowTypes(dt arrow.DataType) (arrow.DataType, error) {
}
type convertToArrow struct {
+ // the ctx stores table property metadata,
+ // for instance, projjson definitions that can be
+ // used when writing
+ ctx context.Context
Review Comment:
Non-blocking: storing a `context.Context` in the `convertToArrow` struct is generally discouraged in Go (contexts should flow as function arguments) and may trip the `containedctx` linter. Since it's only read during `Visit`, passing it through the visit call rather than storing it on the struct would avoid the stored-context pattern.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson read/write support [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#discussion_r3647331841
##########
table/equality_delete_writer.go:
##########
@@ -130,7 +130,10 @@ func (t *Transaction) WriteEqualityDeletes(ctx context.Context, equalityFieldIDs
return nil, err
}
- arrowSc, err := SchemaToArrowSchema(deleteSchema, nil, true, false)
+ arrowSc, err := SchemaToArrowSchemaWithOptions(deleteSchema, ArrowSchemaOptions{
Review Comment:
Non-blocking: this write path gets the projjson option plumbing but no projjson-specific test; a follow-up assertion here (and for position-delta/compaction/txn-rewrite) would guard against regressions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson read/write support [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson write level support [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson read/write support [iceberg-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
C-Loftus commented on PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#issuecomment-5059349132
Thanks very much for your review and that context @zeroshade that makes sense to me and I agree with that approach!
I have reset this branch to have a clean diff to review in my latest commit https://github.com/apache/iceberg-go/pull/1426/commits/6ec8f7ad3357a5e08bbbcb25635b84d017bfd8c1 and then addressed your feedback. Let me know if you have any thoughts with this new approach:
- Reworked projjson CRS handling to pass table properties explicitly through Arrow conversion options instead of context.Context.
- Added public option-based APIs for schema/type conversion while keeping existing APIs as wrappers.
- Write paths now resolve projjson:<property> from table properties and embed the projjson document into GeoArrow metadata.
- Table-aware read paths now recover the original projjson:<property> CRS by matching field ID to the table schema and validating the embedded projjson matches the table property.
- Added these the new options through scan, write, rewrite, position/equality delete, and projection paths.
I have added tests for write metadata, round trip write/read CRS recovery, mismatch errors, missing field ID errors, and unresolved property errors.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson write level support [iceberg-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
C-Loftus commented on code in PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#discussion_r3610927769
##########
table/arrow_utils.go:
##########
@@ -528,6 +528,10 @@ func ensureSmallArrowTypes(dt arrow.DataType) (arrow.DataType, error) {
}
type convertToArrow struct {
+ // the ctx stores table property metadata,
+ // for instance, projjson definitions that can be
+ // used when writing
+ ctx context.Context
Review Comment:
> @zeroshade — you rejected widening the visitor interface on https://github.com/apache/iceberg-go/issues/1230; does the ctx-carries-required-props shape sit right with you, or would you rather see properties passed explicitly? This decision drives most of the rest of the PR, so I'd like to settle it first.
@zeroshade would you be able to give your thoughts on this comment before I address further feedback / wire up the rest of the PR?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson write level support [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#discussion_r3625889630
##########
table/arrow_utils.go:
##########
@@ -528,6 +528,10 @@ func ensureSmallArrowTypes(dt arrow.DataType) (arrow.DataType, error) {
}
type convertToArrow struct {
+ // the ctx stores table property metadata,
+ // for instance, projjson definitions that can be
+ // used when writing
+ ctx context.Context
Review Comment:
Posted the full decision as a top-level comment: https://github.com/apache/iceberg-go/pull/1426#issuecomment-5039317736 — short version: let's resolve `projjson:` via explicit table-properties options rather than `context.Context` (existing exported funcs stay as thin wrappers), and the read path needs to recover `projjson:<key>` before this can merge. Widening the visitor interface is still a no. Thanks for flagging this one to settle first.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson write level support [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#discussion_r3558214926
##########
table/arrow_utils.go:
##########
@@ -528,6 +528,10 @@ func ensureSmallArrowTypes(dt arrow.DataType) (arrow.DataType, error) {
}
type convertToArrow struct {
+ // the ctx stores table property metadata,
+ // for instance, projjson definitions that can be
+ // used when writing
+ ctx context.Context
Review Comment:
This is the big one, and I think it's worth pulling in @zeroshade before we go further — it's a larger design question than the PR itself.
We're using `context.Context` to carry a *required* input (the table properties needed to resolve `projjson:`), not a cross-cutting concern like cancellation or tracing. The practical cost is that correctness now rides on convention instead of the type system: any call site that forgets `WithTableProperties` compiles fine and only fails at runtime, and only when a projjson CRS actually shows up. That's the exact failure mode #1230 was reported for — so as written this relocates the bug class rather than removing it, and the user-visible error ("could not be resolved from table properties") looks identical to the old "not supported yet".
The alternative I'd lean toward is threading properties explicitly — an `iceberg.Properties` argument, or a small `arrowConvertOptions` struct — down through `convertToArrow` / the visitor / `arrowProjectionVisitor`, so "properties missing" becomes a compile-time question. I know `arrowProjectionVisitor.ctx` already sets a ctx-on-the-visitor precedent, so staying consistent with that is defensible too — but then I'd want the redundant `WithTableProperties` calls (they're re-applied in `recordsToDataFiles`/`positionDeleteRecordsToDataFiles` on ctx that often already carries them) collapsed to a single owning layer.
@zeroshade — you rejected widening the visitor interface on #1230; does the ctx-carries-required-props shape sit right with you, or would you rather see properties passed explicitly? This decision drives most of the rest of the PR, so I'd like to settle it first.
(Minor while we're here: the field comment says the ctx "stores" properties — it carries them via `WithTableProperties`, resolved on demand. Worth tightening.)
##########
table/arrow_utils_internal_test.go:
##########
@@ -478,21 +480,134 @@ func TestIcebergCRSToGeoArrowMetadata(t *testing.T) {
}
for _, tc := range cases {
t.Run(tc.crs, func(t *testing.T) {
- meta, err := icebergCRSToGeoArrowMetadata(tc.crs)
+ meta, err := icebergCRSToGeoArrowMetadata(context.Background(), tc.crs)
require.NoError(t, err)
assert.Equal(t, tc.wantCRSType, meta.CRSType)
})
}
})
t.Run("srid:0 maps to an omitted CRS", func(t *testing.T) {
- meta, err := icebergCRSToGeoArrowMetadata("srid:0")
+ meta, err := icebergCRSToGeoArrowMetadata(context.Background(), "srid:0")
require.NoError(t, err)
assert.Empty(t, meta.CRS)
assert.Empty(t, meta.CRSType)
})
}
+func TestTypeToArrowTypeWithContextResolvesProjJSONCRS(t *testing.T) {
+ const projjson = `{"type":"GeographicCRS","name":"WGS 84"}`
+ ctx := internal.WithTableProperties(context.Background(), iceberg.Properties{
+ "geo.crs": projjson,
+ })
+
+ t.Run("geometry", func(t *testing.T) {
+ geom, err := iceberg.GeometryTypeOf("projjson:geo.crs")
+ require.NoError(t, err)
+
+ dt, err := typeToArrowTypeWithContext(ctx, geom, true, false)
+ require.NoError(t, err)
+
+ wkb, ok := dt.(*geoarrow.WKBType)
+ require.True(t, ok)
+
+ meta := wkb.Metadata()
+ assert.Equal(t, geoarrow.CRSTypePROJJSON, meta.CRSType)
+ assert.JSONEq(t, projjson, string(meta.CRS))
+ // when edge is omitted, it should default to planar
+ assert.Equal(t, geoarrow.EdgePlanar, meta.Edges)
+ })
+
+ t.Run("geography", func(t *testing.T) {
+ geog, err := iceberg.GeographyTypeOf("projjson:geo.crs", string(geoarrow.EdgeSpherical))
+ require.NoError(t, err)
+
+ dt, err := typeToArrowTypeWithContext(ctx, geog, true, false)
+ require.NoError(t, err)
+
+ wkb, ok := dt.(*geoarrow.WKBType)
+ require.True(t, ok)
+
+ meta := wkb.Metadata()
+ assert.Equal(t, geoarrow.CRSTypePROJJSON, meta.CRSType)
+ assert.JSONEq(t, projjson, string(meta.CRS))
+ assert.Equal(t, geoarrow.EdgeSpherical, meta.Edges)
+ })
+}
+
+func TestTypeToArrowTypeWithContextProjJSONCRSErrors(t *testing.T) {
+ geom, err := iceberg.GeometryTypeOf("projjson:geo.crs")
+ require.NoError(t, err)
+
+ t.Run("no properties in context", func(t *testing.T) {
+ _, err := typeToArrowTypeWithContext(context.Background(), geom, true, false)
+ require.Error(t, err)
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ require.ErrorContains(t, err, "could not be resolved from table properties")
+ })
+
+ t.Run("missing table property", func(t *testing.T) {
+ ctx := internal.WithTableProperties(context.Background(), iceberg.Properties{})
+ _, err := typeToArrowTypeWithContext(ctx, geom, true, false)
+ require.Error(t, err)
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ require.ErrorContains(t, err, "references missing table property")
+ })
+
+ t.Run("invalid projjson", func(t *testing.T) {
+ ctx := internal.WithTableProperties(context.Background(), iceberg.Properties{
+ "geo.crs": "{",
+ })
+ _, err := typeToArrowTypeWithContext(ctx, geom, true, false)
+ require.Error(t, err)
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ require.ErrorContains(t, err, "is not valid JSON")
+ })
+}
+
+func TestToRequestedSchemaResolvesProjJSONCRSFromContext(t *testing.T) {
+ const projjson = `{"type":"GeographicCRS","name":"WGS 84"}`
+
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ inputSchema := arrow.NewSchema(nil, nil)
+ bldr := array.NewRecordBuilder(mem, inputSchema)
Review Comment:
`TestToRequestedSchemaResolvesProjJSONCRSFromContext` runs `ToRequestedSchema` against an empty file schema and a zero-column record, so it exercises the resolution logic but not a real write. None of the seven call sites this PR added `WithTableProperties` to — `recordsToDataFiles`, `WriteRecords`, `RollingDataWriter`, `PositionDeltaWriter.Close`, `WriteEqualityDeletes`, `ExecuteCompactionGroup` — are touched by a test proving the property actually flows from the table/transaction down to arrow conversion.
The single most valuable test to add: append a record with a projjson-CRS geometry column and confirm the resulting Parquet's Arrow schema carries the right GeoArrow metadata. That's what closes the gap between "resolution works in isolation" and "the write path wires it up" — and it's exactly the coverage that would catch a future call site forgetting `WithTableProperties`. Could we add one going through `recordsToDataFiles`?
##########
table/arrow_utils.go:
##########
@@ -2071,8 +2088,39 @@ func geoArrowMetadataToIcebergType(meta geoarrow.Metadata) (iceberg.Type, error)
var authorityCodeCRS = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9_-]*:[A-Za-z0-9_.-]+$`)
-func icebergCRSToGeoArrowMetadata(crs string) (geoarrow.Metadata, error) {
+// icebergCRSToGeoArrowMetadata converts an Iceberg type's CRS to GeoArrow
+// metadata. It uses the ctx variable to pass through the projjson definitions
+// from the table properties.
+func icebergCRSToGeoArrowMetadata(ctx context.Context, crs string) (geoarrow.Metadata, error) {
lowerCRS := strings.ToLower(crs)
+
+ if strings.HasPrefix(lowerCRS, "projjson:") {
+ propKey := strings.TrimSpace(crs[len("projjson:"):])
+ if propKey == "" {
+ return geoarrow.Metadata{}, fmt.Errorf("%w: projjson CRS missing table property key", iceberg.ErrInvalidSchema)
+ }
+
+ props := tblutils.TablePropertiesFromContext(ctx)
+ if props == nil {
+ return geoarrow.Metadata{}, fmt.Errorf("%w: projjson CRS %q could not be resolved from table properties", iceberg.ErrInvalidSchema, crs)
+ }
+
+ projjson, ok := props[propKey]
+ if !ok || strings.TrimSpace(projjson) == "" {
+ return geoarrow.Metadata{}, fmt.Errorf("%w: projjson CRS %q references missing table property %q", iceberg.ErrInvalidSchema, crs, propKey)
+ }
+
+ raw := json.RawMessage(projjson)
+ if !json.Valid(raw) {
+ return geoarrow.Metadata{}, fmt.Errorf("%w: projjson CRS table property %q is not valid JSON", iceberg.ErrInvalidSchema, propKey)
+ }
+
+ return geoarrow.Metadata{
+ CRS: raw,
+ CRSType: geoarrow.CRSTypePROJJSON,
Review Comment:
We now write a `geoarrow.Metadata` with `CRSType: PROJJSON` and a JSON-object CRS, but `geoArrowCRSToIcebergCRS` on the read side has no case for `meta.CRSType == PROJJSON` in its `checkCRSJSON` branch — it only special-cases the plain-string PROJJSON form. So a table we write here can't be reconstructed back to `projjson:<key>` by our own read path (or by compaction/rewrite, which re-derives schema from the written Parquet). The property key isn't stored in the GeoArrow metadata at all, so the round-trip is fundamentally lossy.
I'd at minimum add an explicit `PROJJSON` case to the `checkCRSJSON` path that returns a clear, distinguishable error rather than falling through to authority/code parsing, and cover the write-then-read behavior with a test so the limitation is documented rather than surprising. If full round-trip is out of scope for this PR that's fine, but I'd want it called out explicitly. wdyt?
(While we're here: the `errPROJJSONStringCRSNotSupported` comment — "which this package does not support yet" — reads stale now that we write the object form; worth a tweak so a future maintainer doesn't read PROJJSON as fully covered.)
##########
table/equality_delete_writer.go:
##########
@@ -27,6 +27,7 @@ import (
"github.com/apache/iceberg-go"
"github.com/apache/iceberg-go/config"
"github.com/apache/iceberg-go/internal"
+ tblutils "github.com/apache/iceberg-go/table/internal"
Review Comment:
`table/internal` gets imported unaliased as `internal` in `transaction.go`/`writer.go` but aliased `tblutils` here (and in `rolling_data_writer.go`, `rewrite_data_files.go`, `position_delta_writer.go`, `arrow_utils.go`) — so `internal.WithTableProperties` and `tblutils.WithTableProperties` are the same function under two names, which is harder to grep/refactor. I'd standardize on `tblutils` for `table/internal` and reserve the bare `internal` name for the root package.
##########
table/arrow_utils.go:
##########
@@ -2071,8 +2088,39 @@ func geoArrowMetadataToIcebergType(meta geoarrow.Metadata) (iceberg.Type, error)
var authorityCodeCRS = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9_-]*:[A-Za-z0-9_.-]+$`)
-func icebergCRSToGeoArrowMetadata(crs string) (geoarrow.Metadata, error) {
+// icebergCRSToGeoArrowMetadata converts an Iceberg type's CRS to GeoArrow
+// metadata. It uses the ctx variable to pass through the projjson definitions
+// from the table properties.
+func icebergCRSToGeoArrowMetadata(ctx context.Context, crs string) (geoarrow.Metadata, error) {
lowerCRS := strings.ToLower(crs)
+
+ if strings.HasPrefix(lowerCRS, "projjson:") {
+ propKey := strings.TrimSpace(crs[len("projjson:"):])
Review Comment:
Not a bug — slicing the original-case `crs` after matching on `lowerCRS` is safe since `"projjson:"` is a fixed 9-byte ASCII prefix, and preserving the key's case is correct. But the case-insensitive-scheme / case-sensitive-key asymmetry is easy to misread as a bug on a skim (the `srid:` branch below does the same). A one-line comment would save the next reader a double-take.
##########
table/arrow_utils.go:
##########
@@ -714,7 +718,12 @@ var _ iceberg.SchemaVisitorPerPrimitiveType[arrow.Field] = convertToArrow{}
// is true, then each field of the schema will contain a metadata key PARQUET:field_id set to
// the field id from the iceberg schema.
func SchemaToArrowSchema(sc *iceberg.Schema, metadata map[string]string, includeFieldIDs, useLargeTypes bool) (*arrow.Schema, error) {
Review Comment:
Because the public entry points hardcode `context.Background()`, an external caller building a custom writer around `SchemaToArrowSchema`/`TypeToArrowType` can never resolve a `projjson:` CRS — they get "could not be resolved from table properties" with no signature to pass the props through, and it's the same error text as the pre-fix state, so it reads like "add the property and it'll work" when there's actually no path to.
#1230 doesn't scope itself to internal writers, so this is a real half-fixed state. I'd either add public `SchemaToArrowSchemaWithContext(ctx, ...)`/`TypeToArrowTypeWithContext(ctx, ...)` mirroring the internal ones, or state plainly in the exported doc comments that the public functions can't resolve `projjson:` references. This is tied to the ctx-vs-explicit-props question above — whichever way that lands should decide the public shape too.
##########
table/rolling_data_writer.go:
##########
@@ -108,6 +108,7 @@ func withFactoryFileSchema(schema *iceberg.Schema) writerFactoryOption {
// configuration derived from the table metadata.
func newWriterFactory(rootLocation string, args recordWritingArgs, meta *MetadataBuilder, taskSchema *iceberg.Schema, targetFileSize int64, opts ...writerFactoryOption) (*writerFactory, error) {
nextCount, stopCount := iter.Pull(args.counter)
+ propCtx := tblutils.WithTableProperties(context.Background(), meta.props)
Review Comment:
`newWriterFactory` builds `propCtx` off `context.Background()`, so any deadline/cancellation/tracing/allocator on the caller's inbound ctx is dropped for schema building and for the option callbacks (`withFactoryFileSchema` gets `propCtx`, not the real request ctx). Benign today since only properties are threaded, but it's a footgun the moment a callee honors cancellation or calls `compute.GetAllocator(ctx)` — which this file family already does elsewhere. All three call sites have a real ctx in scope; I'd thread it into `newWriterFactory` and derive `propCtx` from it.
Related: making `writerFactoryOption` take a ctx forced `withContentType` and `withFactoryEqualityFieldIDs` into `_ context.Context` even though neither uses it — only `withFactoryFileSchema` needs it. If the resolved props/ctx lived on `writerFactory` once at construction instead, that ripple goes away.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson read/write support [iceberg-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
C-Loftus commented on PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#issuecomment-5093274370
@zeroshade your comments should be addressed now.
I added focused coverage for the equality-delete writer path. The test writes an equality delete on a geometry(projjson:...) key and verifies the generated Parquet Arrow schema contains the resolved PROJJSON metadata from table properties. I kept compaction/position-delta out of this test because those route through the existing WriteRecords/schema-conversion paths; happy to add separate follow-up coverage if desired.
I then renamed some of the `internal` references to `tblutils` for standardization as you requested and fixed any merge conflicts. Should be set to review again.
Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson write level support [iceberg-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
C-Loftus commented on PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#issuecomment-4958042729
Thanks for the review @zeroshade ; at your convenience would you be able to give your thoughts on Andrei's comment here https://github.com/apache/iceberg-go/pull/1426#discussion_r3558214926 ?
Namely:
> @zeroshade — you rejected widening the visitor interface on https://github.com/apache/iceberg-go/issues/1230; does the ctx-carries-required-props shape sit right with you, or would you rather see properties passed explicitly? This decision drives most of the rest of the PR, so I'd like to settle it first.
From my understanding, prior discussions in #1230 Andrei said that:
> 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.
I think there is a good argument to pass the data to the `Visit` call as you say here https://github.com/apache/iceberg-go/pull/1426#discussion_r3566791333 but just wanted to be aligned on if I should change the current approach of passing a `ctx`. No worries if approaches need to change, I am happy to make edits here! Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): projjson write level support [iceberg-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
C-Loftus commented on PR #1426:
URL: https://github.com/apache/iceberg-go/pull/1426#issuecomment-5016502671
Hi @zeroshade thanks for your comment. I agree with the feedback although I think I am currently blocked on us aligning on the proper way to pass the metadata. Would you able to provide any thoughts on this? https://github.com/apache/iceberg-go/pull/1426#issuecomment-4958042729 I think we still need to explicitly align on through approach to pass the projjson metadata.
To my understanding Andrei's original comment was to pass the projjson table metadata through a context whereas your follow ups suggested modifying the `VisitGeometry` call directly by adding a new argument. Here is the original alignment question from Andrei: https://github.com/apache/iceberg-go/pull/1426#discussion_r3558214926
I am totally fine with either approach and happy to defer to your preference, I would just like to explicitly align on one option or the other before proceeding and ensure everyone is on the same page. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org