Skip to content

perf: cache Iceberg reflection lookups on the planning path - #5222

Open
andygrove wants to merge 3 commits into
apache:mainfrom
andygrove:perf/iceberg-reflection-cache
Open

perf: cache Iceberg reflection lookups on the planning path#5222
andygrove wants to merge 3 commits into
apache:mainfrom
andygrove:perf/iceberg-reflection-cache

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #5199 (item 2: uncached Class.getMethod throughout the Iceberg reflection paths).

Rationale for this change

Class.getMethod walks a class's public method list and returns a fresh defensive copy of the
Method on every call. Comet's Iceberg paths resolve the same handful of accessors once per file
scan task, and again per partition field and per delete file, so planning a scan over a table with
many files does O(files) reflective lookups that all resolve to the same few methods. Under AQE that
is repeated for every query stage.

IcebergReflection.extractFileLocation is the worst case: it probes for location() on every file
and detects older Iceberg by catching NoSuchMethodException, so on the versions that only have
path() (Iceberg < 1.7, which is what the Spark 3.4 profile builds against) every call constructs an
exception with a stack trace.

Lookup cost, measured in-process against the real org.apache.iceberg.ContentFile interface
(Spark 4.1 / JDK 17, Iceberg 1.11, 200k iterations, best of 5 after warm-up):

lookup before after
getMethod("location") on ContentFile 47.8 ns 7.3 ns
extractFileLocation, location() present 31.7 ns 13.1 ns
extractFileLocation, path()-only version 1830 ns 22 ns

End to end, serializing a Hadoop-catalog table with 8 partitions x 500 files = 4000 file scan tasks
and no delete files (CometIcebergNativeScan.serializePartitions, best of 5 per run, three JVMs):

run 1 run 2 run 3
before 37.9 ms 38.5 ms 38.3 ms
after 17.8 ms 17.3 ms 17.7 ms

Roughly 2.2x. The method caching alone accounts for 38.2 ms -> 20.5 ms; memoizing the per-task
field-id mapping takes it the rest of the way.

CometScanRule.validateIcebergFileScanTasks is unchanged at 3.6-3.7 ms for the same 4000 tasks: its
lookups were already hoisted out of the loop, and only the transform() probe was per task.

What changes are included in this PR?

IcebergReflection gains a resolved-method cache and the lookups now go through it:

  • findMethod / getMethod / findAccessibleMethod / getAccessibleMethod / getDeclaredMethod,
    plus the existing findMethodInHierarchy, all read through the cache. Absent methods are cached as
    misses, which is what removes the per-file exception on path()-only Iceberg versions.
  • The cache is a ClassValue keyed on the class object, so entries are reclaimed with the class and
    a cached Iceberg method never pins a classloader Spark has discarded. Overloads are keyed by
    parameter type, and setAccessible runs once, when a method is first resolved.
  • getFileFormat gains an overload taking an already-loaded ContentFile class, resolving the
    TODO that was there; CometScanRule uses it in the delete-file loop.

In CometIcebergNativeScan.serializePartitions and its per-task helpers:

  • DeleteFile and PartitionSpecParser/PartitionSpec are loaded once per pass instead of per task
    (the PartitionSpecParser.toJson accessor is resolved lazily and passed by name, so a failure to
    resolve it still surfaces as the per-task warning it did before, not an eager failure of the scan).
  • The per-task buildFieldIdMapping is memoized by schema, and the loop-invariant "does the scan
    schema reference field ids the table schema no longer has" check is hoisted to a lazy val.

Behavior is unchanged throughout: getMethod still throws NoSuchMethodException so the existing
catch blocks keep driving version fallbacks, and the cached lookups return the same methods.

How are these changes tested?

Existing coverage: CometIcebergNativeSuite (97 tests), CometFuzzIcebergSuite (10),
CometIcebergRewriteActionSuite (5) and CometIcebergEncryptionSuite (4) all pass.

IcebergReflectionSuite gains unit tests for the cache: that a resolved method is returned by
identity on repeat lookups, that a missing method is cached as a miss and that getMethod still
throws NoSuchMethodException for one, that overloads are distinguished by parameter type, that
findMethodInHierarchy still finds an inherited method, and that extractFileLocation reads
location() when present, falls back to path() when not (repeatedly, so the cached miss is
exercised), and returns None when neither exists.

The probe used for the numbers above was throwaway and is not included.

`Class.getMethod` linearly scans a class's public methods and returns a
fresh `Method` copy per call, and Comet's Iceberg paths resolve the same
handful of accessors once per file scan task, per partition field and per
delete file. `extractFileLocation` additionally probes for `location()` on
every file, so on Iceberg versions that only have `path()` each call built
a `NoSuchMethodException`.

Resolve lookups through a cache in `IcebergReflection`, keyed on the class
via `ClassValue` so entries die with the class rather than pinning a
classloader. Misses are cached too. Memoize the per-task field-id mapping
and hoist the loop-invariant schema comparison out of the serialization
loop.
@andygrove andygrove added this to the 1.1.0 milestone Aug 2, 2026
Fold `setAccessible` into `findMethod` so accessibility is a property of the
resolved method rather than of the call site, dropping the
`findAccessibleMethod` / `getAccessibleMethod` pair and its cache-key
namespace. Share the declared-method resolution between `getDeclaredMethod`
and `findMethodInHierarchy`, and read the cache before `computeIfAbsent` so a
hit does not allocate a mapping function.

Drop the now-unused single-argument `getFileFormat`, and give
`getEqualityFieldIds` the same class-taking shape so its callers stop
reloading `DeleteFile` per delete file.

In the serde, take the `PartitionSpecParser.toJson` accessor as a plain
`Option[Method]` instead of a by-name parameter backed by a `lazy val`, and
reuse the scan-schema field-id mapping the metadata already carries.
@mbutrovich

mbutrovich commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

I'll take a look at this today. I recall our concerns about this the last time it was attempted was maintaining the semantics of: reflection failures during CometScanRule should just trigger a fall back with a message. Reflection failures at serde time must fail loudly and not allow a scan to proceed with e.g., missing delete files (since that would produce wrong results silently). As long as the cache can report to callers why they might get None back from a call (i.e., was it caused by a reflection failure or is the field truly empty) we should be good.

@mbutrovich
mbutrovich self-requested a review August 3, 2026 13:47

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, thanks @andygrove!

…g file class

The previous test resolved `transform()` on a suite-local class. scalac emits
nested classes as public, so the invoke succeeded whether or not
`makeAccessible` had run and only the `isAccessible` flag was really checked.

Build a real `DataFile` instead: its concrete class `GenericDataFile` and the
`BaseFile` that declares its accessors are both package-private in every Iceberg
version Comet builds against, so invoking `path()` from the suite's package
throws `IllegalAccessException` unless the resolved method had access checks
suppressed. Modifier assertions guard against the test going vacuous if those
classes ever become public.

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending CI, thanks @andygrove!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants