You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "karuppayya (via GitHub)" <gi...@apache.org> on 2026/04/03 01:21:21 UTC
[PR] Fix Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
karuppayya opened a new pull request, #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895
## Which issue does this PR close?
Closes #3894.
## Rationale for this change
Fix `NoSuchMethodException` from Iceberg Reflection
## What changes are included in this PR?
Use `findMethodInHierarchy` instead of `getDeclaredMethod` so getTableMetadata and format-version reflection work when `current()` is declared on a superclass (e.g. BaseMetastoreTableOperations for Hive/Glue-style ops).
## How are these changes tested?
- Added Units
- Tested by querying Hive backed Iceberg tables where this issue is present.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#discussion_r3061518475
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:
##########
@@ -228,11 +228,18 @@ object IcebergReflection extends Logging {
val opsMethod = table.getClass.getDeclaredMethod("operations")
opsMethod.setAccessible(true)
val ops = opsMethod.invoke(table)
- val currentMethod = ops.getClass.getDeclaredMethod("current")
- currentMethod.setAccessible(true)
- val metadata = currentMethod.invoke(ops)
- val formatVersionMethod = metadata.getClass.getMethod("formatVersion")
- Some(formatVersionMethod.invoke(metadata).asInstanceOf[Int])
+ findMethodInHierarchy(ops.getClass, "current")
Review Comment:
I think I tried that at some point but it's been at least 6 months, open to trying again.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#discussion_r3048736920
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:
##########
@@ -228,11 +228,18 @@ object IcebergReflection extends Logging {
val opsMethod = table.getClass.getDeclaredMethod("operations")
opsMethod.setAccessible(true)
val ops = opsMethod.invoke(table)
- val currentMethod = ops.getClass.getDeclaredMethod("current")
- currentMethod.setAccessible(true)
- val metadata = currentMethod.invoke(ops)
- val formatVersionMethod = metadata.getClass.getMethod("formatVersion")
- Some(formatVersionMethod.invoke(metadata).asInstanceOf[Int])
+ findMethodInHierarchy(ops.getClass, "current")
Review Comment:
Is current the only method affected? `operations` (a few lines above this) is calling `getDeclaredMethod` also.
Also, are there other catalog implementations that could have similar issues but for other methods?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#discussion_r3059344168
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:
##########
@@ -228,11 +228,18 @@ object IcebergReflection extends Logging {
val opsMethod = table.getClass.getDeclaredMethod("operations")
opsMethod.setAccessible(true)
val ops = opsMethod.invoke(table)
- val currentMethod = ops.getClass.getDeclaredMethod("current")
- currentMethod.setAccessible(true)
- val metadata = currentMethod.invoke(ops)
- val formatVersionMethod = metadata.getClass.getMethod("formatVersion")
- Some(formatVersionMethod.invoke(metadata).asInstanceOf[Int])
+ findMethodInHierarchy(ops.getClass, "current")
Review Comment:
I think we could always call replace all occurences of `getDeclaredMethod` with `findMethodInHierarchy`
Especially, those that assume the superclass's depth seems fragile
```
val tasksMethod = scan.getClass.getSuperclass
.getDeclaredMethod("tasks")
```
```
val filterExpressionsMethod = scan.getClass.getSuperclass.getSuperclass
.getDeclaredMethod("filterExpressions")
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#discussion_r3065568289
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:
##########
@@ -228,11 +228,18 @@ object IcebergReflection extends Logging {
val opsMethod = table.getClass.getDeclaredMethod("operations")
opsMethod.setAccessible(true)
val ops = opsMethod.invoke(table)
- val currentMethod = ops.getClass.getDeclaredMethod("current")
- currentMethod.setAccessible(true)
- val metadata = currentMethod.invoke(ops)
- val formatVersionMethod = metadata.getClass.getMethod("formatVersion")
- Some(formatVersionMethod.invoke(metadata).asInstanceOf[Int])
+ findMethodInHierarchy(ops.getClass, "current")
Review Comment:
Sgtm
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#issuecomment-4315047260
> Fix: treat null scheme as unsupported so these scans fall back.
Rejecting scheme == null also blocks HadoopCatalog local paths (e.g. /tmp/warehouse/...) which have no URI scheme — this caused all CometIcebergNativeSuite tests to fail again.
Instead, added an explicit FileIO type check. (REST catalogs are unaffected(mentioned in the code comment) since the client-side table uses ResolvingFileIO, not InMemoryFileIO (see [RESTSessionCatalog.DEFAULT_FILE_IO_IMPL](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L117)))
Also added `countDeletes() = false` override in the Iceberg diffs for 3.5 (present already for 3.5) since iceberg-rust doesn't expose the delete count metric.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#discussion_r3061497213
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:
##########
@@ -228,11 +228,18 @@ object IcebergReflection extends Logging {
val opsMethod = table.getClass.getDeclaredMethod("operations")
opsMethod.setAccessible(true)
val ops = opsMethod.invoke(table)
- val currentMethod = ops.getClass.getDeclaredMethod("current")
- currentMethod.setAccessible(true)
- val metadata = currentMethod.invoke(ops)
- val formatVersionMethod = metadata.getClass.getMethod("formatVersion")
- Some(formatVersionMethod.invoke(metadata).asInstanceOf[Int])
+ findMethodInHierarchy(ops.getClass, "current")
Review Comment:
@mbutrovich wdyt? Is there a potential performance impact here if we replace `getDeclaredMethod` with `findMethodInHierarchy`?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#issuecomment-4277405668
@mbutrovich ping!
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#issuecomment-4202778821
cc: @andygrove @mbutrovich @parthchandra
can any of you help review?
Also looks like workflow needs maintainer approval to progress(But remember that it used to work without this)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#discussion_r3065562375
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:
##########
@@ -228,11 +228,18 @@ object IcebergReflection extends Logging {
val opsMethod = table.getClass.getDeclaredMethod("operations")
opsMethod.setAccessible(true)
val ops = opsMethod.invoke(table)
- val currentMethod = ops.getClass.getDeclaredMethod("current")
- currentMethod.setAccessible(true)
- val metadata = currentMethod.invoke(ops)
- val formatVersionMethod = metadata.getClass.getMethod("formatVersion")
- Some(formatVersionMethod.invoke(metadata).asInstanceOf[Int])
+ findMethodInHierarchy(ops.getClass, "current")
Review Comment:
Thinking this over, I feel we can defer changing everything over to use `findMethodInHierarchy` until we hit an issue.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#issuecomment-4226865355
We've exposed a latent bug, perhaps. In `CometScanRule.validateIcebergFileScanTasks`, `uri.getScheme` returns `null` for the relative paths returned by `InMemoryCatalog`. We check
```
if (scheme != null && !supportedSchemes.contains(scheme)) {
unsupportedSchemes += scheme
}
```
which allows such paths and this later fails.
@mbutrovich, any thoughts on how to handle this?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#issuecomment-4281344180
Thanks for improving the Iceberg support, @karuppayya! I took a look at this today:
The `findMethodInHierarchy` fix is correct, but it widens the set of scans Comet intercepts. Previously, `getDeclaredMethod("current")` failed for `InMemoryCatalog`'s operations class (where `current()` is inherited), causing fallback to Spark. Now the reflection succeeds and Comet runs the scan natively.
The Iceberg CI failures (`TestSparkReaderDeletes`) happen because Comet's native reader (iceberg-rust/arrow-rs) doesn't expose the `number of row deletes applied` metric that these tests assert on. That metric is an Iceberg Java reader concept.
The root cause is `CometScanRule.validateIcebergFileScanTasks` line 809:
```scala
if (scheme != null && !supportedSchemes.contains(scheme))
```
`InMemoryCatalog` returns relative paths where `URI.getScheme` is `null`. The null check lets them through. Fix: treat null scheme as unsupported so these scans fall back.
```scala
if (scheme == null || !supportedSchemes.contains(scheme))
```
Same fix needed for the delete file scheme check on line 851.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895#issuecomment-4309225277
I’ll keep kicking CI :)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Iceberg reflection for current() on TableOperations hierarchy [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich merged PR #3895:
URL: https://github.com/apache/datafusion-comet/pull/3895
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org