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