andygrove commented on code in PR #5222:
URL: https://github.com/apache/datafusion-comet/pull/5222#discussion_r3713598860
##########
spark/src/test/scala/org/apache/comet/iceberg/IcebergReflectionSuite.scala:
##########
@@ -63,4 +63,90 @@ class IcebergReflectionSuite extends AnyFunSuite {
assert(metadata.isDefined)
assert(metadata.get.isInstanceOf[TableMetadata])
}
+
+ test("findMethod resolves a method once and returns the cached instance") {
+ val first = IcebergReflection.findMethod(classOf[Schema], "columns")
+ val second = IcebergReflection.findMethod(classOf[Schema], "columns")
+ assert(first.isDefined)
+ assert(first.get.getName == "columns")
+ // Class.getMethod hands back a fresh copy per call; the cache must not.
+ assert(first.get eq second.get)
+ }
+
+ test("an absent method is a cached miss, and getMethod still throws for it")
{
+ assert(IcebergReflection.findMethod(classOf[Schema],
"noSuchAccessor").isEmpty)
+ assert(IcebergReflection.findMethod(classOf[Schema],
"noSuchAccessor").isEmpty)
+ assertThrows[NoSuchMethodException] {
+ IcebergReflection.getMethod(classOf[Schema], "noSuchAccessor")
+ }
+ }
+
+ test("findMethod distinguishes overloads by parameter type") {
+ val byId = IcebergReflection.findMethod(classOf[Schema], "findField",
classOf[Int])
+ val byName = IcebergReflection.findMethod(classOf[Schema], "findField",
classOf[String])
+ assert(byId.isDefined && byName.isDefined)
+ assert(byId.get ne byName.get)
+
+ val schema = new Schema(Types.NestedField.required(7, "id",
Types.IntegerType.get()))
+ val fieldById = byId.get.invoke(schema,
Integer.valueOf(7)).asInstanceOf[Types.NestedField]
+ val fieldByName = byName.get.invoke(schema,
"id").asInstanceOf[Types.NestedField]
+ assert(fieldById.name() == "id")
+ assert(fieldByName.fieldId() == 7)
+ }
+
+ test("findMethodInHierarchy finds an inherited method and caches it") {
+ val first =
IcebergReflection.findMethodInHierarchy(classOf[StubTableOperations], "current")
+ val second =
IcebergReflection.findMethodInHierarchy(classOf[StubTableOperations], "current")
+ assert(first.isDefined)
+ // current() is declared on BaseMetastoreTableOperations, not on the stub
itself.
+ assert(first.get.getDeclaringClass ==
classOf[BaseMetastoreTableOperations])
+ assert(first.get eq second.get)
+
assert(IcebergReflection.findMethodInHierarchy(classOf[StubTableOperations],
"nope").isEmpty)
+ }
+
+ test("extractFileLocation reads location() when the class has one") {
+ val file = new LocationFile("s3://bucket/data/f.parquet")
+ assert(
+ IcebergReflection.extractFileLocation(classOf[LocationFile], file) ==
+ Some("s3://bucket/data/f.parquet"))
+ }
+
+ test("extractFileLocation falls back to path() on Iceberg versions without
location()") {
+ val file = new PathOnlyFile("s3://bucket/data/f.parquet")
+ // Called twice: the second call reads the cached "location() is absent"
answer.
+ assert(
+ IcebergReflection.extractFileLocation(classOf[PathOnlyFile], file) ==
+ Some("s3://bucket/data/f.parquet"))
+ assert(
+ IcebergReflection.extractFileLocation(classOf[PathOnlyFile], file) ==
+ Some("s3://bucket/data/f.parquet"))
+ }
+
+ test("extractFileLocation returns None when the class exposes neither
accessor") {
+ assert(IcebergReflection.extractFileLocation(classOf[Object], new
Object).isEmpty)
+ }
+
+ test("a resolved method has access checks suppressed") {
+ // Iceberg's bound terms and file impls are instances of package-private
classes, so the
+ // accessors Comet resolves on them have to be accessible before they can
be invoked.
+ val method = IcebergReflection.findMethod(classOf[HiddenTransform],
"transform")
+ assert(method.isDefined)
+ assert(method.get.isAccessible)
+ assert(method.get.invoke(new HiddenTransform).toString == "identity")
+ }
Review Comment:
Good catch on the flag-vs-behavior distinction: the `invoke` on the next
line proved nothing, since `HiddenTransform` compiles to a public class.
`private[iceberg]` wouldn't get there either, for two reasons. scalac emits
qualified-private (and plain `private`) nested classes as *public* JVM classes,
since the JVM has no equivalent of qualified private. And even with
package-private bytecode, the caller here is
`org.apache.comet.iceberg.IcebergReflectionSuite`, which is in the same runtime
package as the stub, so the access check would pass anyway.
Fixed in ba95bdf9e by using a real Iceberg file instead of a synthetic
class: `DataFiles.builder(...).build()` returns a `GenericDataFile`, whose
accessors are declared on `BaseFile`, and both are package-private in 1.5.2
(Spark 3.4), 1.8.1 (3.5) and 1.11.0 (4.1). Invoking `path()` on one from
Comet's package is exactly the situation `makeAccessible` exists for.
`Modifier.isPublic` assertions on the concrete and declaring classes keep the
test from going vacuous if Iceberg ever makes them public.
Verified by stubbing `makeAccessible` to a no-op:
```
- a resolved method has access checks suppressed *** FAILED ***
java.lang.IllegalAccessException: class
org.apache.comet.iceberg.IcebergReflectionSuite
cannot access a member of class org.apache.iceberg.BaseFile with modifiers
"public"
```
and 9/9 passing with it restored.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
