feat: fuse the typed Dataset map sandwich into a Comet projection - #5714
feat: fuse the typed Dataset map sandwich into a Comet projection#5714andygrove wants to merge 2 commits into
Conversation
Collapses the SerializeFromObject / MapElements / DeserializeToObject island that `ds.map(f)` produces into a projection over the deserializer's child, so a typed map no longer forces a Spark fallback in the middle of an otherwise native plan. The projection converts through the ordinary CometProjectExec path and the fused tree routes through the JVM codegen dispatcher, so there is no proto or native change. Neither object operator can run natively on its own: one outputs and the other consumes a raw JVM object reference, which has no Arrow representation. Fusing works because CometBatchKernelCodegen.canHandle only type-checks the root and the bound references, so the object may exist strictly inside the tree. Spark already builds the fused expression -- the rule mirrors MapElementsExec.doConsume and the two doConsume methods whole-stage codegen chains around it. With more than one output column the rule emits two stacked projections: an inner one producing a single CreateNamedStruct, and an outer one extracting the fields with GetStructField. That is a correctness requirement, not tidiness: N separate projection expressions would each carry their own copy of the closure Invoke and each become its own kernel, calling user code N times per row where Spark calls it once. The struct is tagged FORCE_DISPATCH so it compiles into one kernel, and subexpression elimination inside that kernel collapses the shared Invoke back to a single call. Off by default. The rewrite pays off when the typed operation sits between native operators and can cost a little when it is at the top of the plan, so flipping the default needs benchmarks first. Part of apache#5572. Closes apache#5710.
Fixes a real limitation found while reviewing: `canHandle`'s `spark.sql.codegen.maxFields` gate counted `BoundReference` *occurrences*, but the fused struct puts the shared deserializer in every field on purpose, so a 12-column typed map counted 12 + 12*12 = 156 fields against a default cap of 100 and silently declined to fuse. WSCG gates on the operator's schema, where each column contributes once, and the kernel likewise emits one typed field and one getter per ordinal -- so count distinct ordinals. Verified: a 12-column record declined before, fuses now, and is pinned by a new test. Reuse and duplication: - Extract `CometScalaUDF.canDispatch` / `bindForDispatch`. The rule was re-deriving the bind-then-canHandle gate and had already drifted: it omitted the `RuntimeReplaceable` unwrap, so the plan-time prediction could disagree with what the serde does. - Move `FORCE_DISPATCH` to `QueryPlanSerde`, which reads it. Matches the convention every other behavior tag follows (SKIP_COMET_SCAN_TAG, SKIP_COMET_BROADCAST_TAG, COMET_UNSAFE_PARTIAL all live on their reader), and documents that the tag bypasses the per-expression policy layer. - Check the tag ahead of the version shim, not inside the `orElse`, so the override is genuinely unconditional -- the shim matches on Invoke/StaticInvoke, exactly what this rule synthesizes. Simplification: fold the dispatch gate and tag-set into one `forceDispatch` so the two facts cannot travel separately; hoist the CSE precondition above the output-count branch; `AttributeSet(fused) -- child.outputSet` for the dangling check; rename `declineQuietly` to `decline` (it is the opposite of quiet -- it writes an EXPLAIN-visible reason) and route the dispatcher-disabled case through it so that reason reaches EXPLAIN too. Altitude: correct the pre-pass comment, which claimed a technical necessity that does not exist -- `convertNode` only tags `DeserializeToObjectExec`, it never replaces it, so the sandwich is intact when the walk reaches the parent. Records that doing it in `convertNode` would additionally expose the profitability signal this rewrite lacks. Move the pre-pass above `normalizePlan` so the synthesized projections get the same NaN / -0.0 normalization as every other `ProjectExec`. Tests: replace a tautological schema assertion (Dataset.schema comes from the analyzed plan, which a physical rule cannot change) with one on the executed plan's output attributes; pin both decline messages with `checkSparkAnswerAndFallbackReason`; collapse the four-case operator collector to `ObjectConsumerExec | ObjectProducerExec`; stop building the failure clue on the happy path; add `withTypedRecs` / `withParquetRoundTrip` fixtures.
|
Pushed a review pass. One finding was a real limitation rather than a cleanup, so flagging it here:
Also worth a reviewer's attention, and deliberately not changed here: This rewrite could live in The rest is cleanup: extracted On tests: one assertion was vacuous — 17 tests, green on the default profile and |
Which issue does this PR close?
Closes #5710. Part of #5572.
Rationale for this change
ds.map(f)drops a three-operator island into the middle of an otherwise native plan, and every operator in it falls back:It cascades past the island.
ds.map(f).groupBy("b").count()loses the aggregate and the exchange too — 2 of 8 eligible operators accelerated.Neither object operator can be handled on its own.
DeserializeToObjectExecoutputs a singleObjectTypeattribute andSerializeFromObjectExecconsumes one;ObjectTypeis outsideQueryPlanSerde.supportedDataTypeand outsideCometBatchKernelCodegen.isSupportedDataType, because a JVM object reference cannot live in an Arrow vector.CodegenDispatchFallback's self-type isCometExpressionSerde[_], so there is no operator-level dispatch hook to mix in either.Fusing the sandwich works, though, for two reasons.
canHandleonly type-checks the rootdataTypeand everyBoundReference— intermediate nodes are never inspected — so the object may exist strictly inside the tree while the outer boundary stays ordinary SQL data. And Spark already builds the fused expression:MapElementsExec.doConsumeconstructsInvoke(Literal.create(func, ObjectType(funcClass)), funcName, outputObjectType, child.output, propagateNull = false), so the closure has a first-class Catalyst representation. The rule just does statically what whole-stage codegen does by chaining the threedoConsumes.What changes are included in this PR?
RewriteTypedDatasetMap(new), run as a pre-pass inCometExecRule._applybeforetransform, since the bottom-up conversion would otherwise reachDeserializeToObjectExecand fall the island back before anything could fuse it. It rewrites the sandwich into a projection over the deserializer's child. The projection then converts through the ordinaryCometProjectExecpath and the fused tree routes through the JVM codegen dispatcher — no proto change and no native change.Multiple output columns.
emitJvmCodegenDispatchemits oneJvmScalarUdfper expression and gets one Arrow vector back, butSerializeFromObjecthas N serializers that all share oneInvoke. Converting them separately would produce N kernels and call the user closure N times per row where Spark calls it once — a real behavioural difference for a side-effecting closure, not just a slowdown. So for N > 1 the rule emits two stacked projections: an inner one producing a singleCreateNamedStruct, and an outer one extracting the fields withGetStructField. Both node types already have serdes, andCometBatchKernelCodegenOutputalready mapsStructTypetoStructVector.CometScalaUDF.FORCE_DISPATCH. The struct needs to compile into one kernel, butCreateNamedStructhas a perfectly good native serde that would convert each field independently and put us back at N kernels. ATreeNodeTagchecked at the top ofexprToProtoInternalforces whole-subtree dispatch. I used a tag rather than a Comet-specificExpressionsubclass deliberately: the rewritten plan stays built entirely from stock Spark expressions, so it still executes correctly if the enclosing operator falls back to Spark for an unrelated reason.Scope. Only
MapElementsExec, including a chain of them (ds.map(f).map(g)leaves two adjacentMapElementsunder one Serialize/Deserialize pair).MapPartitionsExec,FlatMapGroupsExecandCoGroupExecconsume iterators or groups, so no per-row expression exists.AppendColumnsExecis per-row but widens the schema; left for a follow-up. Typed filters never produce this sandwich — Catalyst lowers them to aFilterExecover anInvoke, which #5692 already dispatches.Declines rather than guesses when the serializer has a non-
Aliaselement, reads an unexpected bound reference, produces a dangling attribute reference,canHandlerefuses the tree, the dispatcher is disabled, or (for N > 1) subexpression elimination is off. Each records a fallback reason soEXPLAINsays why instead of showing the bare "not supported".Off by default (
spark.comet.exec.typedDatasetMap.enabled). The rewrite pays off when the typed operation sits between native operators; when it is at the top of the plan (ds.map(f).collect()) the gain is roughly nil and could be slightly negative, since the kernel writes into Arrow only for something to read rows straight back out. Flipping the default needs benchmarks, and it is worth checking whetherRevertNativeForTransitionHeavyStagesalready covers that shape.How are these changes tested?
New
CometTypedDatasetSuite, 16 tests, registered in bothpr_build_linux.ymlandpr_build_macos.yml. Green on the default profile and on-Pspark-3.5; main and test code compile onspark-3.4,spark-3.5,spark-4.0and the default. No regressions inCometCodegenSuite(86),CometCodegenSourceSuite(60),CometExpressionSuite(141),CometExecRuleSuite(29) orCometCoverageStatsSuite.Beyond
checkSparkAnswerAndOperatoron single-column, multi-column, wide (decimal / string /Option), nested-struct-and-array, chained-map and JavaMapFunctionshapes, the tests that carry the most weight:closure runs exactly once per row with multiple output columns— a JVM-static counter asserts 50 calls for 50 rows. This is the test that would catch the N-kernel regression the struct wrapper exists to prevent.fusion unblocks the aggregate and shuffle above it— asserts thegroupBy().count()case is fully native, i.e. the cascade is actually fixed.output schema is unchanged by the rewrite— compares the schema tree with the flag on and off, guarding the nullability reasoning behindGetStructFieldover a non-nullableCreateNamedStruct.AssertNotNull inside the fused kernel still raises like Spark— returning null for a non-nullable product is an error in Spark; the serializer'sassertnotnullhas to survive the fuse or Comet would silently emit a null row. The stack trace confirms it fires fromSpecificCometBatchKernel.subExpr_0$, which incidentally also confirms CSE hoisted the sharedInvoke.decimal overflow in the serializer is caught before the Arrow write— I raised this on Fuse the SerializeFromObject / MapElements / DeserializeToObject sandwich into a Comet projection instead of falling back #5710 as the Route unrecognizedStaticInvokeandInvokethrough the codegen dispatcher instead of falling back #5575-shaped risk: an encoder-declareddecimal(38,18)receiving a wider value that Spark nulls at row materialization but the kernel'sDecimalVectorwrite might not. It does not apply. The encoder's serializer already wraps the value inCheckOverflow, so the fused tree raises under ANSI and nulls under non-ANSI exactly where Spark does, ahead of the write. The test pins both directions. I verified the same is true on the pre-existing plain-ScalaUDFdecimal path.mapPartitionsleft alone.Not covered, and worth a reviewer's attention: no benchmark numbers yet, which is why the flag is off. Nested (non-top-level) case classes carry a
NewInstanceouterPointerclosure over the enclosing instance that closure serialization would drag along —ScalaUDFhas the same exposure, but I have not written a test for it.