andygrove opened a new issue, #5258:
URL: https://github.com/apache/datafusion-comet/issues/5258
## 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 #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.
--
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]
