fix: ignore structural tags when lifting expression coverage

[sunchao]

Which issue does this PR close?

No issue is automatically closed. This is a follow-up to the structural-expression tag filtering associated with #5229, which is already closed.

Rationale for this change

Comet's extended explain reports how much of a query runs natively or through the JVM codegen dispatcher. Its expression-coverage counts come from metadata recording the expressions used by the plan. Stale names in that metadata can inflate coverage counts or add misleading dispatch labels, making it harder to understand what Comet actually accelerated.

Spark's rewrites can copy a tagged expression's metadata onto the process-wide Literal.TrueLiteral singleton. Later queries in that JVM share the same literal. Existing filtering ignores tags found directly on literals, but decimal promotion leaves another path for them to escape.

For example, suppose an earlier query left greaterthanorequal coverage on the shared literal. Consider this illustrative projection, where amount is DECIMAL(10, 2):

SELECT named_struct('flag', true, 'sum', amount + amount) AS value
FROM payments;

There is no comparison in this projection. However, decimal promotion rebuilds its expression tree to add overflow checking. The current coverage lift collects tags from that rebuilt tree, including the stale literal tag, and copies them onto the projection's original output alias. An alias can legitimately hold coverage collected from its child expressions, so subsequent filtering accepts the copied name. The query's native coverage now includes greaterthanorequal even though the query does not contain it. Codegen-dispatch coverage can be contaminated in the same way.

This makes diagnostics depend on earlier queries in the JVM and can produce misleading coverage counts or unstable explain output. It does not corrupt query results.

What changes are included in this PR?

The coverage lift now applies the existing structural-node filtering before copying native and codegen-dispatch names back to the original expression. Stale tags on literals and other nodes that cannot legitimately own coverage are discarded before they can be attached to an alias.

Genuine coverage still needs to survive the rewrite. In particular, decimal promotion introduces a CheckOverflow expression that is absent from the original tree; its coverage must still reach the original owner. The change preserves that behavior and keeps aliases as valid recipients. It changes expression metadata and the existing regression test, without changing query results, execution routing, or fallback decisions.

How are these changes tested?

The expanded CometCodegenSuite regression seeds both coverage-tag categories on the shared literal, checks that decimal promotion excludes them while retaining real checkoverflow coverage, and checks an unrelated plan. The seeded tags deliberately stand in for earlier-query contamination; this is not a claim that the test reproduces the entire preceding query history.

At head 37004235, the Spark 4.0 / JDK 21 expression job passed this regression and the existing expression-coverage tests. Local JVM validation had been blocked by dependency resolution; the passing runtime evidence comes from CI.

CI on 37004235 is now green, with 63 passing checks and 9 skips. The Spark 3.5 shuffle rerun and the Iceberg 1.11 Spark, extensions, and runtime jobs passed. The review follow-up also compiled the full Spark 4.1.3 JVM reactor locally and passed five coverage/singleton tests; disabling the coverage lift made the new positive dispatcher regression fail. These local tests reused a previously built OSS native library and did not rebuild native code. CI for the review follow-up commit is not yet confirmed.

[andygrove]

Thanks for chasing this down. Routing the lift through collectExprTagValues looks like the right place to me, because once a stale name has landed on an Alias the roll-up has no way to tell it apart from a real one.

I checked the write side and I agree the filter is lossless. isStructuralExpr at QueryPlanSerde.scala:1002 already stops the serde from tagging an Attribute, BoundReference, or Literal, so anything this filter discards can only have arrived by copyTagsFrom. CheckOverflow is not structural, so the coverage the lift exists for still survives.

One question outside the diff. Does liftFallbackReasons just below have the same exposure? It walks every node in the rebuilt tree the way liftCoverageTags used to, and CometExecRule.rollUpFallbackReasons does not filter either, so a stale reason branded onto Literal.TrueLiteral would surface on an unrelated operator. The branding path looks like the one the test comment already describes, where PlanDynamicPruningFilters swaps a DynamicPruningSubquery for the singleton, and a subquery expression is very likely to be carrying a Comet fallback reason at that point. I do not think it belongs in this PR, partly because the fix cannot be symmetric there. Line 993 does legitimately tag literals with fallback reasons, so isNeverTagged would be the wrong filter. It also seems worse than a coverage count, since a spurious reason on the operator is enough to satisfy reportUnexplainedFallback and would hide a genuinely missing reason under COMET_STRICT_FALLBACK_REASONS. Could you file an issue and link it here so it does not get lost?

CI is green now, including the Spark 3.5 shuffle job and both Iceberg 1.11 jobs, so the last paragraph of the description is stale and worth refreshing.

[andygrove]

What makes this filter safe is that isStructuralExpr at line 1002 never lets the serde tag an Attribute, BoundReference, or Literal in the first place, so CometExplainInfo.isNeverTagged can only ever discard copied tags. That invariant is now load-bearing for this fix, but the two lists live in different files under different names and neither comment mentions the other. Could isNeverTagged be derived from isStructuralExpr minus Alias, or failing that, could each comment name the other and state the subset relationship? Otherwise someone adding a node type to just one of them either reopens this path or quietly deletes real coverage.

[sunchao]

Updated in 95c744c. Added reciprocal comments on isStructuralExpr and isNeverTagged: the read filter is the write-side structural set minus Alias, because rewritten-child coverage is lifted onto the original alias. I also narrowed the wording to coverage/info tags and explicitly excluded FALLBACK_REASONS from that invariant; literals can legitimately carry those reasons.

[andygrove]

Would it be better to split this out into its own test? It asserts something quite different from the rest of the test it is in, which is about the dynamic-pruning plan not reporting the planted name, and the test name and comment only describe that second half. A separate test named for the lift, something like "the coverage lift ignores stale tags on the shared TrueLiteral", would point straight at the mechanism when it fails, and it would sit next to "expression coverage stats survive the decimal promotion rewrite", which is the positive case for the same code path.

Related question. Is there a way to drive this through a real plan the way the neighbouring tests do, or does planting on the singleton force the direct exprToProto call? I ask because this is the only place in these suites that builds a Catalyst tree by hand, so if a plan-level version is workable it would be more consistent and would cover the roll-up and the explain rendering too.

[sunchao]

Updated in 95c744c. I kept the seeded-singleton probe with its plan-level check so the two phases share one setup and guaranteed cleanup. The direct call pins the exact Literal.TrueLiteral identity and the original Alias; normal SQL optimization can change that shape. The neighboring decimal test already covers plan-level coverage, and the second half of this test covers DPP roll-up and extended explain. There are also existing direct Catalyst probes in this suite for BoundReference and map/array expressions.

[andygrove]

This change routes CODEGEN_DISPATCH_EXPRS through the structural filter for the first time, and both of the new assertions for it are negative. Would it be worth adding a positive case, where an expression inside a promoted decimal tree really is routed through the JVM codegen dispatcher, checking that its name still reaches the original owner? Without one, a future change to isNeverTagged could drop genuine dispatch coverage across a decimal rewrite and these tests would still pass.

[sunchao]

Updated in 95c744c. Added codegen dispatch coverage survives the decimal promotion rewrite. It places Hypot above a cast of decimal addition, so promotion rebuilds both Hypot and its alias. The test checks the emitted JvmScalarUdf dispatcher class, verifies that the original Hypot has neither the dispatch marker nor its coverage tag, and requires the original alias to receive hypot. This passed locally on Spark 4.1.3. As a mutation check, removing only liftCoverageTags(newExpr, expr) made this new assertion fail with None did not contain Set("hypot").

[sunchao]

Updated in 95c744c4c.

Addressed the coverage follow-up in 95c744c: the two structural predicates now document their relationship, and a positive dispatcher regression proves a name created only on the promoted copy reaches the original alias. The full Spark 4.1.3 JVM reactor compiled; all five selected coverage/singleton tests passed. Removing only the lift made the new regression fail. Local execution reused a previously built OSS native library; no native changes are in this follow-up.

The fallback-reason provenance follow-up is tracked in #5499. It covers stale-reason propagation and possible masking of the strict check while preserving legitimate literal fallback reasons. I kept the proposed DPP contamination origin explicitly unproven: Spark wraps TrueLiteral, and the rule ordering alone does not reproduce singleton branding.

I also refreshed the stale final CI paragraph. The earlier head's reruns are green; the new commit's CI remains to be confirmed.

[sunchao]

Update, August 27 at 19:20 UTC: The retry completed successfully on the unchanged PR head 95c744c4c. Workflow attempt 2 passed.

The original Spark 4.2 / JDK 17 shuffle job failed before the Java modules compiled or tests ran: Maven Central returned HTTP 429 while resolving org.junit:junit-bom:5.10.1 through the RAT plugin dependencies. It was a dependency setup failure, not a shuffle-test failure.

GitHub initially rejected the retry while the parent workflow was running. Once that workflow completed, the rerun was accepted at 19:05 UTC and subsequently passed. No code change was needed for this failure.