fix: ignore structural tags when lifting expression coverage - #5471
Open
sunchao wants to merge 1 commit into
Open
fix: ignore structural tags when lifting expression coverage#5471sunchao wants to merge 1 commit into
sunchao wants to merge 1 commit into
Conversation
sunchao
marked this pull request as ready for review
August 26, 2026 18:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.TrueLiteralsingleton. 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
greaterthanorequalcoverage on the shared literal. Consider this illustrative projection, whereamountisDECIMAL(10, 2):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
greaterthanorequaleven 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
CheckOverflowexpression 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
CometCodegenSuiteregression seeds both coverage-tag categories on the shared literal, checks that decimal promotion excludes them while retaining realcheckoverflowcoverage, 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.Full CI is not yet confirmed at the time of this update. The original Spark 3.5 shuffle job failed downloading a Maven dependency, and the Iceberg 1.11 job reached its six-hour limit after a test stopped progressing. Their reruns are in progress; the Iceberg hang's cause remains unresolved.