Re: [PR] [AURON #2375] Fix Iceberg changelog scan field-id projection [auron]

["weiqingy (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@auron.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org