andygrove commented on PR #5204: URL: https://github.com/apache/datafusion-comet/pull/5204#issuecomment-5152672103
Follow-up notes from a self-review pass, for reviewer context: **Coverage.** The interning applies to plans serialized via `CometNativeExec.convertBlock` — the main scan-rooted path, which is what the benchmark numbers measure. These paths still serialize inline `sql_text` and are **not** covered here: `CometNativeShuffleWriter` (per task), `CometIcebergNativeScanExec`'s `convertBlock` override, `CometNativeWriteExec`, `CometTakeOrderedAndProjectExec`, and the bare-`Expr` Parquet filter path (which has no root operator to hang a pool on, so it will always need the inline form). Routing the operator paths through the interner as well is a small change — move the `intern` call into `CometExec.serializeNativePlan` — but it adds a tree walk to the per-task shuffle-write path, so I left it out of this PR rather than ship it untested. Worth a follow-up. **`sourceKey` altitude.** The scan plan-data key is a content hash over `Message.toString` of several `NativeScanCommon` fields, computed independently on the driver and the executor. That is what broke when interning changed the context encoding, and it will be fragile against any future encoding change. This PR collapses the two copies into one function so it can only be got wrong in one place, but the deeper fix is to carry an explicit key in the proto the way `IcebergScanCommon.scan_hash_code` already does — which would also delete `stripQueryContexts`' only caller. Tracked as a follow-up under the audit epic. -- 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]
