You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "C-Loftus (via GitHub)" <gi...@apache.org> on 2026/07/27 14:51:16 UTC
[PR] fix(parquet/pqarrow): Change interface name for arrow type on read for extension types [arrow-go]
C-Loftus opened a new pull request, #1028:
URL: https://github.com/apache/arrow-go/pull/1028
### Rationale for this change
#969 added an interface for a custom arrow type on read for arrow extension types. This allows a type like geometry to be resolved to the underlying arrow physical type like byte array.
However, I went to implement this in geoarrow-go today and upon further investigation I think the interface name is slightly confusing. The interface `ExtensionParquetLogicalType` doesn't actually include the `ParquetLogicalType` method but rather it intended to be used to specify the arrow type from parquet on read. As such I have renamed it.
I figure it makes sense to solidify this before creating a new arrow-go release and adding it to iceberg-go whereupon it will be much harder to change.
I changed the interface name to indicate that it is one side of the read/write path for extension types. I think this is a bit more clear but if I am misunderstanding and it might be used in both read and write, then maybe it doesn't make sense.
### What changes are included in this PR?
Comments and a change in the name of an interface. This interface is not yet in a published release but is on main. Since its only been a few days, I think it is reasonable to change this.
### Are these changes tested?
Yes
### Are there any user-facing changes?
Yes. Change in interface name. But no changes in behavior.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): Change interface name for arrow type on read for extension types [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1028:
URL: https://github.com/apache/arrow-go/pull/1028
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): Change interface name for arrow type on read for extension types [arrow-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
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 adding read support for an extension type why an extension type needs itself 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): Change interface name for arrow type on read for extension types [arrow-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
C-Loftus commented on code in PR #1028:
URL: https://github.com/apache/arrow-go/pull/1028#discussion_r3658316362
##########
parquet/pqarrow/schema.go:
##########
@@ -121,24 +121,24 @@ 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.
Review Comment:
Changed to `on write` since it makes it clearer that it is one side of the read/write path for arrow <-> parquet but worth aligning if there are any cases in which this would be used beyond writing
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): Change interface name for arrow type on read for extension types [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1028:
URL: https://github.com/apache/arrow-go/pull/1028#issuecomment-5093976315
@C-Loftus why the change to draft?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/pqarrow): Change interface name for arrow type on read for extension types [arrow-go]
Posted by "C-Loftus (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org