zeroshade commented on code in PR #969:
URL: https://github.com/apache/arrow-go/pull/969#discussion_r3632075751
##########
arrow/datatype_extension.go:
##########
@@ -82,6 +82,18 @@ func GetExtensionType(typName string) ExtensionType {
return nil
}
+// RegisteredExtensionTypes returns a snapshot of all registered extension
types.
+// The returned types are in an unspecified order.
+func RegisteredExtensionTypes() []ExtensionType {
+ registry := getExtTypeRegistry()
+ var out []ExtensionType
+ registry.Range(func(_, value any) bool {
+ out = append(out, value.(ExtensionType))
+ return true
+ })
+ return out
+}
Review Comment:
This adds permanent exported API to the core `arrow` package, so worth being
deliberate before it ships.
It's also the first place in the codebase that *enumerates* the registry —
everywhere else does name-based point lookups (`GetExtensionType`, e.g. the
UUID read path in `arrowFromFLBA` and IPC extension deserialization in
`arrow/ipc/metadata.go`). Enumerate-and-ask is what introduces the ordering
ambiguity flagged at the call site below.
If the only consumer is pqarrow's reverse lookup, consider a predicate
helper instead, e.g. `FindRegisteredExtensionType(pred func(ExtensionType)
bool) ExtensionType`, which keeps the `Range` bounded to one call, allows
early-exit on first match, and avoids allocating a full snapshot slice. If you
keep the slice form, the "unspecified order" doc is accurate and good to have.
##########
parquet/pqarrow/schema.go:
##########
@@ -541,6 +551,27 @@ func arrowFromByteArray(logical schema.LogicalType)
(arrow.DataType, error) {
}
}
+// arrowExtensionFromParquetLogicalType asks registered extension types whether
+// they can represent the provided Parquet logical type with the given storage
+// type, falling back to the storage type when none opt in.
+func arrowExtensionFromParquetLogicalType(logical schema.LogicalType,
storageType arrow.DataType) (arrow.DataType, error) {
+ for _, extType := range arrow.RegisteredExtensionTypes() {
Review Comment:
`arrow.RegisteredExtensionTypes()` is backed by `sync.Map`, whose `Range`
order is unspecified. If two registered extensions both return non-nil from
`ArrowTypeFromParquet` for the same logical type (plausible for geo: a user's
custom type plus a future canonical `geoarrow.*`), the winner is arbitrary and
can differ across processes — reads aren't reproducible.
Contrast the existing UUID mapping in `arrowFromFLBA`, which is
deterministic via `GetExtensionType("arrow.uuid")`. Suggest either iterating a
name-sorted snapshot and taking the first match, or having extensions register
an explicit logical-type -> extension mapping so resolution is well-defined.
##########
parquet/pqarrow/schema.go:
##########
@@ -541,6 +551,27 @@ func arrowFromByteArray(logical schema.LogicalType)
(arrow.DataType, error) {
}
}
+// arrowExtensionFromParquetLogicalType asks registered extension types whether
+// they can represent the provided Parquet logical type with the given storage
+// type, falling back to the storage type when none opt in.
+func arrowExtensionFromParquetLogicalType(logical schema.LogicalType,
storageType arrow.DataType) (arrow.DataType, error) {
+ for _, extType := range arrow.RegisteredExtensionTypes() {
+ converter, ok := extType.(ExtensionParquetLogicalType)
+ if !ok {
+ continue
+ }
+
+ typ, err := converter.ArrowTypeFromParquet(logical, storageType)
+ if err != nil {
+ return nil, err
+ }
Review Comment:
Returning the error here propagates up through `getArrowType` ->
`nodeToSchemaField`, which aborts the entire `FromParquet` conversion. So a
single third-party extension that returns an error for "not my CRS/logical
type" breaks reading of *every* geometry/geography column in the process,
including files unrelated to it.
Combined with the non-deterministic iteration order above, whether a read
hits that error can depend on which extension `Range` visits first — the same
file may read fine or fail across runs.
Recommend: reserve a non-nil `error` for genuinely fatal conditions, keep
"not mine" as `(nil, nil)` (the interface doc and the test double already
follow this), and either `continue` past an errored extension or only surface
the error if no other extension matches.
##########
parquet/pqarrow/schema.go:
##########
@@ -541,6 +551,27 @@ func arrowFromByteArray(logical schema.LogicalType)
(arrow.DataType, error) {
}
}
+// arrowExtensionFromParquetLogicalType asks registered extension types whether
+// they can represent the provided Parquet logical type with the given storage
+// type, falling back to the storage type when none opt in.
+func arrowExtensionFromParquetLogicalType(logical schema.LogicalType,
storageType arrow.DataType) (arrow.DataType, error) {
+ for _, extType := range arrow.RegisteredExtensionTypes() {
+ converter, ok := extType.(ExtensionParquetLogicalType)
+ if !ok {
+ continue
+ }
+
+ typ, err := converter.ArrowTypeFromParquet(logical, storageType)
+ if err != nil {
+ return nil, err
+ }
+ if typ != nil {
+ return typ, nil
+ }
+ }
+ return storageType, nil
Review Comment:
Good that this falls back to the storage type when nothing opts in. Note
this also changes existing behavior: a Geometry/Geography byte-array column now
reads back as `Binary` even when no extension is registered, whereas before it
hit the `default` in `arrowFromByteArray` and errored.
That new default path isn't covered — both new tests register an extension
first. Please add a case asserting Geometry-with-no-registered-extension maps
to `Binary`, so the fallback (and the fact that it no longer errors) is locked
in.
##########
parquet/pqarrow/schema.go:
##########
@@ -129,6 +129,13 @@ type ExtensionCustomParquetType interface {
ParquetLogicalType() schema.LogicalType
}
+// ExtensionParquetLogicalType is an interface that Arrow ExtensionTypes may
+// implement to specify how a Parquet LogicalType maps back to an Arrow
+// ExtensionType when converting a Parquet schema to an Arrow schema.
+type ExtensionParquetLogicalType interface {
+ ArrowTypeFromParquet(logical schema.LogicalType, storageType
arrow.DataType) (arrow.ExtensionType, error)
+}
Review Comment:
Naming/symmetry nit on exported API: the write side is
`ExtensionCustomParquetType.ParquetLogicalType()` and this read side is
`ExtensionParquetLogicalType.ArrowTypeFromParquet()`. A round-trippable
extension has to implement both, and the two interface names are easy to
confuse. Consider a symmetric pair (e.g. `...ToParquet` / `...FromParquet`) or
folding both directions into a single interface. Harder to change once released.
--
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]
