IcebergReflection: Option helpers cannot distinguish an absent accessor from a failed reflective call

[andygrove]

Is your feature request related to a problem or challenge?

IcebergReflection's Option-returning helpers collapse two very different outcomes into None:

• the Iceberg version on the classpath does not declare this accessor, which is a normal, expected version difference; and
• the accessor exists but the reflective call threw, which is a real failure.

extractFileLocation (spark/src/main/scala/org/apache/comet/iceberg/IcebergReflection.scala:266-280) is the clearest case. It probes for location(), falls back to path(), and wraps the whole thing in catch { case _: Exception => None }, so a None means either "this is Iceberg < 1.7" or "the invoke blew up". getFileFormat (:298-309) has the same shape, and the pattern repeats across the object.

That matters because the two paths that call these want opposite things. CometScanRule can fall back to Spark with a message, so a quiet None is fine there. Serde runs after the plan is committed to the native scan, so a None that was really a failure has to fail the query. Today the serde callers cannot tell, so they either turn every None into a throw (CometIcebergNativeScan.scala:288, :1027-1031 — correct, but the error cannot say what actually went wrong) or they pick a default and carry on.

This was raised in review on #5222: as long as the reflection layer can report to callers why they might get None back, the loud-at-serde-time / quiet-at-planning-time split can be enforced properly.

Describe the potential solution

Give the helpers a way to distinguish the two. Options worth weighing:

• Return Either[ReflectionFailure, Option[T]], or a small ADT with Found / NotDeclared / Failed cases, so callers pattern-match on the reason.
• Keep Option for "not declared" and let genuine invocation failures propagate as exceptions, so the quiet planning-path callers catch them explicitly and serde callers get them for free. Narrower change, and it matches what findMethod already does after perf: cache Iceberg reflection lookups on the planning path #5222: it returns None only for NoSuchMethodException and lets everything else through.

The second is the smaller diff and fits the existing grain, but it moves the burden onto every planning-path caller to add a catch. Worth deciding before the two companion issues are fixed, since both of them need this distinction to do the right thing.

Additional context

Companion issues cover the delete-file fields that fall back to wrong defaults on reflection failure, and the schema/metadata helpers that return empty collections. This one is the enabler for both: neither can cleanly separate "genuinely absent" from "reflection failed" until the helpers report it.

Note that the method cache added in #5222 does not itself introduce this ambiguity. findMethod caches a miss only for NoSuchMethodException, which is the legitimate version-difference signal; the conflation is in the higher-level helpers and predates that change.

Related: #5256 (delete-file defaults), #5257 (schema/metadata helpers returning empty collections).

[mbutrovich]

CometScanRule.scala:743-744 calls IcebergReflection.getEqualityFieldIds(deleteFileClass, deleteFile) on the same DeleteFile that serde later reflects on again, and that helper's catch (IcebergReflection.scala:730 case _: Exception => in the PR branch) returns an empty list for a NoSuchMethodException and for a genuine invoke failure alike. So a genuine invoke failure at planning time does not produce a fallback reason, it produces an empty equality-id list, which CometScanRule reads as "this is a position delete" and lets through to the native path unchanged. This is a live instance of the ambiguity, not a hypothetical one, and it argues for the second option in the issue: keep Option/empty-collection for "not declared" and let a genuine invoke failure propagate. findMethod already does this split (None only for NoSuchMethodException); extending it to getEqualityFieldIds and the other helpers would fix the false pass-through at CometScanRule.scala:743 and give serde a real exception to catch, instead of requiring two independent fixes.