apache / apache/datafusion-comet

Track provenance of fallback reasons copied through shared expression nodes

Open
#5,499 0 comments 0 reactions 0 assignees View on GitHub
area:expressions bug priority:low
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

### Describe the bug

Fallback reasons have a separate provenance problem from the coverage tags fixed in #5471. If a stale `FALLBACK_REASONS` tag reaches a shared expression such as `Literal.TrueLiteral`, an unrelated failed conversion can lift it onto its original expression owner. Operator roll-up also collects reasons from every descendant. A nonempty stale reason can then satisfy `reportUnexplainedFallback`, hiding an otherwise unexplained fallback when `spark.comet.explain.fallback.strict.enabled` is enabled.

This is a source-confirmed propagation path conditional on a stale tag; it is not a reproduced sequence of two SQL queries. The proposed dynamic partition pruning origin still needs investigation. Spark 4.1.3's `PlanDynamicPruningFilters` replaces the subquery with a `DynamicPruningExpression(TrueLiteral)` wrapper, and its non-AQE rule runs before Comet conversion. That source alone does not establish that the shared literal receives a fallback reason.

### Steps to reproduce

Add focused tests that preserve and restore the singleton's previous tags:

1. Seed a distinctive stale fallback reason on `Literal.TrueLiteral`.
2. Convert an expression containing that literal and decimal arithmetic, so decimal promotion rebuilds the surrounding tree. Force the conversion to decline and check which reasons are lifted onto the original owner.
3. Exercise operator roll-up and the strict fallback check with an operator that otherwise lacks an explanation. Verify that the planted reason cannot masquerade as an explanation for the current conversion.
4. Also test legitimate literal fallback reasons, including an unsupported literal type and explicitly disabled literal support.

These are regression targets, not a claim that all four tests already exist or that the singleton contamination producer has been reproduced.

### Expected behavior

An expression or operator should report reasons belonging to its current conversion, without diagnostics from unrelated queries. The strict check should continue to detect an otherwise missing reason. Legitimate reasons attached to literals must remain visible: applying the coverage-only `isNeverTagged` filter to fallback reasons would discard real diagnostics.

### Additional context

The relevant paths at the reviewed head are [the failure-only fallback lift](https://github.com/apache/datafusion-comet/blob/370042350eba68bf720b31f5550f491c94554ba6/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala#L843-L882), [operator roll-up and the strict check](https://github.com/apache/datafusion-comet/blob/370042350eba68bf720b31f5550f491c94554ba6/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala#L799-L838), and [legitimate literal fallback reasons](https://github.com/apache/datafusion-comet/blob/370042350eba68bf720b31f5550f491c94554ba6/spark/src/main/scala/org/apache/comet/serde/literals.scala#L101-L124).

The potential DPP origin should be checked against [Spark 4.1.3's actual replacement](https://github.com/apache/spark/blob/v4.1.3/sql/core/src/main/scala/org/apache/spark/sql/execution/dynamicpruning/PlanDynamicPruningFilters.scala#L54-L81), not assumed from coverage-tag behavior. Related earlier work: #5229 and #5230. Keep this follow-up separate from #5471, which only changes coverage metadata.

Contributor guide

Open the contributing guide

Research direction

Start with the failure-only fallback lift in spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala, then inspect operator roll-up and the strict check in CometExecRule.scala and literal handling in literals.scala. Add focused tests that preserve and restore Literal.TrueLiteral tags, cover decimal conversion, roll-up, strict checking, unsupported literals, and disabled literal support; verify stale reasons cannot explain unrelated fallbacks while legitimate literal reasons remain visible.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala
Domain
backend, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.