andygrove opened a new issue, #5200: URL: https://github.com/apache/datafusion-comet/issues/5200
## Describe the bug `QueryPlanSerde` attaches a `QueryContext` to **every** expression it serializes, and `QueryContext.sql_text` is the *full* SQL text of the query. The same string is therefore embedded once per expression in the serialized native plan. On a modest 2-table join + aggregate query (778-char SQL), 71 of the 90 `Expr` nodes in the plan carried a copy of the query text: ``` total serialized plan bytes = 58445 Expr nodes in plan = 90 Expr nodes with query_context = 71 query_context serialized bytes = 56227 (96.2% of plan) distinct sql_text strings = 1 plan bytes with ctx stripped = 1803 <- 32x smaller per-task proto round trip full = 0.153 ms per-task proto round trip slim = 0.063 ms <- 2.4x faster ``` TPC-DS queries are 1-3 KB of SQL, so the ratio gets worse, not better. ## Where it comes from - `QueryPlanSerde.exprToProtoInternal` ends with a `.map` that calls `extractQueryContext(expr)` for every converted expression; `aggExprToProto` does the same for aggregates. - `QueryContext.sql_text` in `native/proto/src/proto/expr.proto` is declared as the full query text, and `SQLQueryContext.sqlText` is what gets copied into it. ## Why it costs more than just bytes 1. **Plan size.** The `Array[Byte]` is captured by `CometExecRDD` and shipped in the task binary for the stage. 2. **Per-task JVM protobuf round trip.** Whenever a plan contains a native scan (i.e. essentially every scan-rooted Comet stage), `CometExecRDD.compute` does `Operator.parseFrom(serializedPlan)` then `PlanDataInjector.injectPlanData` then `serializeOperator` — **per task**. Measured at 0.153 ms vs 0.063 ms with the duplication removed. At 10k tasks that is ~1.5 s of JVM protobuf churn per stage, ~60% of it avoidable. 3. **Native side.** `PhysicalPlanner::create_expr` and `create_agg_expr` do `ctx_proto.sql_text.clone()` into a fresh `String` and register it in `QueryContextMap`, per expression, per task. That is 71 identical heap strings per plan per task, held for the lifetime of the query. Note `QueryContext.sql_text` is already `Arc<String>` internally, so the sharing was anticipated — it is just never realised because each proto entry decodes to its own `String`. ## Proposed fix: intern the SQL text into a pool Truncating to just the relevant fragment is *not* an option: `QueryContext::format_summary` prints the entire `sql_text` (that is what Spark's `== SQL (line N, position M) ==` block shows), so the full text is genuinely needed for the message. The duplication is the problem, not the content. - Add `repeated string sql_text_pool` to `Operator`, populated only on the root operator of a serialized native block. - Add `optional int32 sql_text_idx` to `QueryContext`; when set, `sql_text` is left empty. - Intern as a post-pass in `CometNativeExec.convertBlock()`, which is the point where the full operator tree for a block is in hand. Doing it there means none of the ~90 serde call sites change and no plan-scoped mutable state has to be threaded through `exprToProto`. - Keep `sql_text` working as a fallback when `sql_text_idx` is absent, so the paths that serialize `nativeOp` directly without going through `convertBlock()` (e.g. `CometNativeWriteExec`) keep working unchanged. - On the native side, resolve `sql_text_idx` against the pool and share one `Arc<String>` across all contexts that reference it, which also removes the per-expression `String` clone. Error messages are unchanged; this is purely a wire-format deduplication. ## Steps to reproduce Plan any SQL query with Comet enabled, take `CometNativeExec.serializedPlanOpt.plan.get`, parse it with `OperatorOuterClass.Operator.parseFrom`, and walk the tree summing `Expr.getQueryContext.getSerializedSize` against the total byte count. ## Expected behavior The serialized plan should contain each distinct SQL text once, not once per expression. ## Additional context Found while doing a broader planner / serde / optimizer-rule performance audit: https://github.com/apache/datafusion-comet/issues/5199 -- 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]
