mbutrovich commented on code in PR #5222:
URL: https://github.com/apache/datafusion-comet/pull/5222#discussion_r3704568817
##########
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:
The test "a resolved method has access checks suppressed" does not exercise
what its name and the surrounding comment claim. `HiddenTransform` (line 149)
is declared with no access modifier, which Scala compiles to a public JVM
class. `Method.isAccessible()` reports whether `setAccessible(true)` was
called, not whether the method actually needed it. A public method on a public
class is invocable via reflection without `setAccessible`, so this test would
pass even if `makeAccessible` did nothing. Declaring `HiddenTransform`
package-private (for example `private[iceberg] class HiddenTransform`) would
make `method.get.invoke(...)` on line 135 throw `IllegalAccessException`
without the suppressed check, so the test would verify the behavior it names
instead of only checking a flag.
--
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]
