C-Loftus commented on code in PR #1028:
URL: https://github.com/apache/arrow-go/pull/1028#discussion_r3659111416
##########
parquet/pqarrow/schema.go:
##########
@@ -121,24 +121,26 @@ func (sm *SchemaManifest) GetFieldIndices(indices []int)
([]int, error) {
}
// ExtensionCustomParquetType is an interface that Arrow ExtensionTypes may
implement
-// to specify the target LogicalType to use when converting to Parquet.
+// to specify the target LogicalType to use when converting to Parquet on
write.
//
// The PrimitiveType is not configurable, and is determined by a fixed mapping
from
// the extension's StorageType to a Parquet type (see getParquetType in
pqarrow source).
type ExtensionCustomParquetType interface {
ParquetLogicalType() schema.LogicalType
}
-// ExtensionParquetLogicalType is an interface that Arrow ExtensionTypes may
+// ExtensionCustomArrowReadType 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.
+// ExtensionType when converting a Parquet schema to an Arrow schema on read.
+// The receiver is the registered extension type being asked whether it can
+// represent the Parquet logical type with the given storage type; callers must
+// use the returned extension type because it may be a distinct instance
carrying
+// per-column parameters derived from the Parquet logical type.
Review Comment:
I added this comment here since I realized that it might be confusing to a
user creating a new extension type why an extension type needs to return a new
extension type struct and it can't just use the existing one after passing it
in and seeing no error.
I used the phrase `per-column parameters derived from the Parquet logical
type.` rather than “metadata” because the values are part of the Parquet
LogicalType object passed to ArrowTypeFromParquet, such as CRS or geography
algorithm fields. “Metadata” could be confused with Arrow field metadata or
ARROW:extension:metadata, which this hook does not receive directly.
##########
parquet/pqarrow/schema.go:
##########
@@ -121,24 +121,26 @@ func (sm *SchemaManifest) GetFieldIndices(indices []int)
([]int, error) {
}
// ExtensionCustomParquetType is an interface that Arrow ExtensionTypes may
implement
-// to specify the target LogicalType to use when converting to Parquet.
+// to specify the target LogicalType to use when converting to Parquet on
write.
//
// The PrimitiveType is not configurable, and is determined by a fixed mapping
from
// the extension's StorageType to a Parquet type (see getParquetType in
pqarrow source).
type ExtensionCustomParquetType interface {
ParquetLogicalType() schema.LogicalType
}
-// ExtensionParquetLogicalType is an interface that Arrow ExtensionTypes may
+// ExtensionCustomArrowReadType 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.
+// ExtensionType when converting a Parquet schema to an Arrow schema on read.
+// The receiver is the registered extension type being asked whether it can
+// represent the Parquet logical type with the given storage type; callers must
+// use the returned extension type because it may be a distinct instance
carrying
+// per-column parameters derived from the Parquet logical type.
//
// ArrowTypeFromParquet should return (nil, nil) if the logical type does not
// map to the extension type. It should return (nil, err) if the logical type
is
-// recognized but cannot be converted into a valid extension type. If a
-// non-nil extension type is returned, that type is used and any previous
-// conversion errors from other extension types are ignored.
Review Comment:
While I am making edits on comments, I removed this here and moved it to
`arrowExtensionFromParquetLogicalType` since the comment on ignoring errors is
moreso on control flow internal to iceberg-go's extension type resolution
process and not the specifically this method.
--
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]
