fix: Iceberg reflection for current() on TableOperations hierarchy#3895
fix: Iceberg reflection for current() on TableOperations hierarchy#3895karuppayya wants to merge 2 commits intoapache:mainfrom
Conversation
mbutrovich
changed the title
karuppayya
force-pushed
the
fix-mehthod-hierarchy
branch
2 times, most recently
from
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.
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
karuppayya
force-pushed
the
fix-mehthod-hierarchy
branch
from
904c10f to
c602b73
Compare
|
We've exposed a latent bug, perhaps. In which allows such paths and this later fails. |
3 participants
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?