weiqingy commented on code in PR #2376:
URL: https://github.com/apache/auron/pull/2376#discussion_r3522206982
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -211,6 +211,31 @@ object IcebergScanSupport extends Logging {
}
val (fileSchema, partitionSchema) = schemas.get
+ val fieldIdsByName =
Review Comment:
This block — field-id extraction, rename/drop detection, the two asserts,
and the `supportedFormat` line at `:284` — now repeats about 25 lines from
`planFileScan` (`:121-144`, `:187-191`), differing only in which util method
each calls. Worth noting this bug itself came from the two paths drifting
apart: the changelog path shipped with `Map.empty` while the file-scan path
already passed field-ids. Would it be worth factoring the shared portion into
one helper parameterized by the two `scan → …` lookups, so a future field-id
change can't miss one path again? Genuinely open — if you'd rather keep the two
paths explicit for readability, that's a reasonable call too.
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/spark/sql/auron/iceberg/IcebergScanSupport.scala:
##########
@@ -256,7 +281,9 @@ object IcebergScanSupport extends Logging {
}
val format = formats.headOption.getOrElse(FileFormat.PARQUET)
- if (format != FileFormat.PARQUET && format != FileFormat.ORC) {
+ val supportedFormat =
Review Comment:
Small consistency thing: the file-scan copy of this `supportedFormat` line
has a comment just above it (`:185-186`) explaining why a top-level rename/drop
makes older ORC files unsafe for native matching, but that comment didn't come
across to the changelog copy. Worth mirroring it here so the ORC branch reads
the same on both paths?
##########
thirdparty/auron-iceberg/src/main/scala/org/apache/iceberg/spark/source/AuronIcebergSourceUtil.scala:
##########
@@ -37,8 +39,23 @@ object AuronIcebergSourceUtil {
expectedSchema.columns().asScala.map(field => field.name() ->
field.fieldId()).toMap
}
+ def expectedFieldIdsForChangelogScan(scan: AnyRef): Map[String, Int] = {
+ val expectedSchema =
+ FieldUtils.readField(scan, "expectedSchema",
true).asInstanceOf[org.apache.iceberg.Schema]
Review Comment:
The file-scan path reads the schema and table through the public
`expectedSchema()` / `table()` methods (`:38`, `:49`), while the changelog path
reaches them by string-keyed reflection into `SparkChangelogScan`
(`FieldUtils.readField(scan, "expectedSchema", true)` here and `"table"` at
`:54`). Since those are Iceberg-internal field names with no compile-time
check, an Iceberg version that renames or restructures the field would slip
through silently. The caller does guard both reads with `try/NonFatal → return
None` (`IcebergScanSupport.scala:214-230`), so the worst case is a quiet
fallback rather than a crash — which is a good safety net. Does
`SparkChangelogScan` expose any public accessor we could use instead, or is
reflection the only door in? If it's the only option, would a one-line note on
the field-name assumption help whoever does the next Iceberg bump?
--
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]
