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: [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]
