Copilot commented on code in PR #12900:
URL: https://github.com/apache/gluten/pull/12900#discussion_r3863062986
##########
gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/TransformerApi.scala:
##########
@@ -74,11 +75,11 @@ trait TransformerApi {
def packPBMessage(message: Message): Any
- /** Packs Iceberg column initial defaults into a backend-specific read
extension. */
+ /** Packs Iceberg field identities and initial defaults into a
backend-specific read extension. */
def packIcebergReadExtension(
- fieldIds: util.Map[String, Integer],
+ fieldIds: util.List[IcebergFieldId],
initialDefaults: util.Map[String, String]): Any = {
- throw new UnsupportedOperationException("Iceberg initial-default reads are
not supported")
+ throw new UnsupportedOperationException("Iceberg field-ID reads are not
supported")
}
Review Comment:
The default `UnsupportedOperationException` message says only "Iceberg
field-ID reads are not supported", but this method now covers both field
identities and initial defaults. If a backend calls this path without
overriding it (or triggers it with defaults-only), the message can be
misleading for debugging.
##########
gluten-iceberg/src/main/scala/org/apache/gluten/execution/IcebergScanTransformer.scala:
##########
@@ -70,10 +71,10 @@ case class IcebergScanTransformer(
GlutenIcebergSourceUtil.getInitialDefaults(scan)
private lazy val icebergFieldIds =
- if (icebergInitialDefaults.isEmpty) {
- new JHashMap[String, Integer]()
- } else {
+ if (BackendsApiManager.getSettings.supportIcebergFieldIdRead()) {
GlutenIcebergSourceUtil.getFieldIds(scan)
+ } else {
+ new JArrayList[IcebergFieldId]()
}
Review Comment:
`icebergFieldIds` is now computed only when `supportIcebergFieldIdRead()` is
true. However, `IcebergLocalFilesNode.toProtobuf()` still calls
`packIcebergReadExtension(fieldIds, initialDefaults)` when *either* `fieldIds`
or `initialDefaults` is non-empty, and
`VeloxTransformerApi.packIcebergReadExtension` requires field IDs to be present
when `initialDefaults` is non-empty (it throws if the field ID is missing).
This makes the feature flags independently configurable in a way that can lead
to runtime failures if a backend ever supports initial-default reads without
also enabling field-id reads.
To keep the API robust, compute field IDs whenever initial-defaults are
being packed (or enforce the invariant that initial-default support implies
field-id support).
--
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]
