Skip to content

fix: ignore structural tags when lifting expression coverage - #5471

Open
sunchao wants to merge 1 commit into
apache:mainfrom
sunchao:codex/coverage-tag-isolation
Open

fix: ignore structural tags when lifting expression coverage#5471
sunchao wants to merge 1 commit into
apache:mainfrom
sunchao:codex/coverage-tag-isolation

Conversation

@sunchao

@sunchao sunchao commented Aug 26, 2026

Copy link
Copy Markdown
Member

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.

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.

@sunchao
sunchao marked this pull request as ready for review August 26, 2026 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant