fix: Iceberg reflection for current() on TableOperations hierarchy#3895
fix: Iceberg reflection for current() on TableOperations hierarchy#3895mbutrovich merged 6 commits intoapache:mainfrom
Conversation
433832e to
904c10f
Compare
|
cc: @andygrove @mbutrovich @parthchandra |
| val metadata = currentMethod.invoke(ops) | ||
| val formatVersionMethod = metadata.getClass.getMethod("formatVersion") | ||
| Some(formatVersionMethod.invoke(metadata).asInstanceOf[Int]) | ||
| findMethodInHierarchy(ops.getClass, "current") |
There was a problem hiding this 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?
There was a problem hiding this 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")
There was a problem hiding this comment.
@mbutrovich wdyt? Is there a potential performance impact here if we replace getDeclaredMethod with findMethodInHierarchy?
There was a problem hiding this comment.
I think I tried that at some point but it's been at least 6 months, open to trying again.
There was a problem hiding this comment.
Thinking this over, I feel we can defer changing everything over to use findMethodInHierarchy until we hit an issue.
904c10f to
c602b73
Compare
|
We've exposed a latent bug, perhaps. In which allows such paths and this later fails. |
|
@mbutrovich ping! |
|
Thanks for improving the Iceberg support, @karuppayya! I took a look at this today: The The Iceberg CI failures ( The root cause is if (scheme != null && !supportedSchemes.contains(scheme))
if (scheme == null || !supportedSchemes.contains(scheme))Same fix needed for the delete file scheme check on line 851. |
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). Add IcebergReflectionSuite with a BaseMetastoreTableOperations stub and BaseTable. Made-with: Cursor
b2c6e5f to
5d5f8bf
Compare
5d5f8bf to
cb61b4f
Compare
|
I’ll keep kicking CI :) |
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)) Also added |
mbutrovich
left a comment
There was a problem hiding this comment.
I have minor style nits but don't want to send you through CI again. Thanks @karuppayya!
Which issue does this PR close?
Closes #3894.
Rationale for this change
Fix
NoSuchMethodExceptionfrom Iceberg ReflectionWhat changes are included in this PR?
Use
findMethodInHierarchyinstead ofgetDeclaredMethodso getTableMetadata and format-version reflection work whencurrent()is declared on a superclass (e.g. BaseMetastoreTableOperations for Hive/Glue-style ops).How are these changes tested?