dwsmith1983 opened a new pull request, #5615:
URL: https://github.com/apache/datafusion-comet/pull/5615

   ## Which issue does this PR close?
   
   No dedicated issue. #5200 fixed the size of the serialized plan; this 
addresses the per-task work done on those bytes.
   
   ## Rationale for this change
   
   The serialized plan bytes are identical for every partition of a stage, yet 
every task parsed the full operator tree from bytes, re-derived the scan's 
source key (stringifying the schema and filter lists, which turns out to be the 
single most expensive step for wide schemas), and re-parsed the scan's common 
message, all before injecting its own partition data. That cost scales with 
plan size times partition count and lands hardest on large scan plans.
   
   ## What changes are included in this PR?
   
   Three bounded per-executor LRU caches, 16 entries each. The parsed base plan 
is cached keyed on byte content, so an executor parses a stage's tree once 
instead of once per task; the parsed `NativeScanCommon` is cached the same way 
the Iceberg injector already caches its common; and the source key gets a memo 
that rides protobuf's reference-identity fast path once the base plan instance 
is shared. Injection itself stays per task, since partition data genuinely 
differs, and the injected tree is never cached, so per-partition file lists 
cannot leak across tasks (there is a test asserting the shared common is 
reference-equal while the file lists diverge). Cache misses compute outside any 
lock, and two threads racing a cold key both end up holding the same instance, 
first insert wins.
   
   A larger follow-up was considered and set aside: shipping the base plan to 
native once per executor and merging partition data there would also remove the 
per-task reserialize and native decode, but injection is a ServiceLoader SPI 
implemented by out-of-tree modules, so moving the merge native would break that 
extension point. Noted for later rather than folded in here.
   
   Measured per-task cost (parse plus key derivation plus reserialize, 5000 
iterations after warmup): a 100-column scan plan goes from roughly 274-380us to 
44-73us, and a 1000-column plan from roughly 2.0-2.5ms to 0.55-0.93ms.
   
   ## How are these changes tested?
   
   Eight new tests in PlanDataInjectorSuite (hit and miss behavior, distinct 
plans staying separate, eviction plus rerun, eight-thread concurrency, cold-key 
race adopting one instance across 200 barrier-synchronized trials, 
shared-common reference equality with per-partition file isolation, and the 
memo matching a fresh derivation), alongside the existing six. The end-to-end 
paths run through CometScanWithPlanDataSuite (5), CometNativeReaderSuite (54), 
CometExecSuite (142), and CometNativeShuffleSuite (40), all green. Spotless and 
scalastyle clean.
   


-- 
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]

Reply via email to