hhr293 commented on PR #58424: URL: https://github.com/apache/spark/pull/58424#issuecomment-5611420685
> Thanks for the PR, @hhr293! > > The rewrite holds up. I traced it independently and then ran it in a review worktree at this head: twelve adversarial shapes all agree with the flag off, and the 31 new tests pass. What I tried was tuple `IN` projecting both sides of one equi pair, `IN` in the SELECT list, `NOT IN` over pattern A2, A2 with the top-level Project removed as a no-op, A2 over a LEFT OUTER outer join, an asymmetric filter on the neq column, a filter on the equi key that only becomes symmetric via `InferFiltersFromConstraints`, a wrapper projecting a subset of the group-by keys, a nested `InSubquery`, and the same `InSubquery` twice in one plan. I also confirmed the motivating case works at this head: with the flag on, the rewrite fires on both of Q95's `IN` subqueries over `TPCDSBase`'s tables. > > Already raised, so not repeating: the cost gate ([r3939100184](https://github.com/apache/spark/pull/58424#discussion_r3939100184)), the source boundary ([r3939100186](https://github.com/apache/spark/pull/58424#discussion_r3939100186)), moving the shape match ahead of `isRepeatablePlan` ([r3939100192](https://github.com/apache/spark/pull/58424#discussion_r3939100192)), the flag's promotion criterion ([r3939100190](https://github.com/apache/spark/pull/58424#discussion_r3939100190)), the extra tests ([r3939104821](https://github.com/apache/spark/pull/58424#discussion_r3939104821)) and the `Cast` string-to-timestamp hole ([r3963602271](https://github.com/apache/spark/pull/58424#discussion_r3963602271)). > > One measurement for the source-boundary thread. Same schema, same data, same query, only the storage changed: the rewrite fires on `parquet` and does not fire on `orc`, `json`, `csv`, or a `CACHE TABLE`d Parquet table. > > ### Blocking > * **1.** _Missing JIRA ID:_ the PR left draft on 2026-09-07 and the title dropped `[Draft]` without gaining a ticket, so it no longer matches Spark's `[SPARK-xxxxx][COMPONENT]` format. [SPARK-41416](https://issues.apache.org/jira/browse/SPARK-41416) already describes this exact rewrite with the same Q95 example, and its attempt [#38951](https://github.com/apache/spark/pull/38951) was closed by the stale bot in 2023 without a single review comment. Retitle to `[SPARK-41416][SQL] ...`, or open a new ticket and link it to that one. > > ### Non-blocking > * **2.** _Test fixtures are never dropped:_ the suite leaves 19 tables and 4 temp views behind. `withTable` / `withTempView` also stop a failing test from handing a half-populated table to the next one. [[inline: `sql/core/src/test/scala/org/apache/spark/sql/execution/RewriteSelfJoinInequalityToAggregateSuite.scala:86`](https://github.com/apache/spark/pull/58424#discussion_r3966743760)] > > ### Alternatives > * **3.** _What do the leaf and expression allowlists buy?:_ a question rather than a request to restructure. The Parquet-exact check reads as a repeatability proof, and I cannot see the property it proves that ORC or JSON lack. [[inline: `sql/core/src/main/scala/org/apache/spark/sql/execution/RewriteSelfJoinInequalityToAggregate.scala:684`](https://github.com/apache/spark/pull/58424#discussion_r3966743770)] > > ### Minor > * **4.** _`canonicalizeWrapper` drops `Alias.explicitMetadata`_ while its sibling helper keeps it. [[inline: `sql/core/src/main/scala/org/apache/spark/sql/execution/RewriteSelfJoinInequalityToAggregate.scala:171`](https://github.com/apache/spark/pull/58424#discussion_r3966743784)] Thanks for the thorough validation, especially for checking the adversarial cases and confirming that the rewrite fires on both Q95 IN subqueries. For the new comments: JIRA ID Good catch. I'll retitle the PR to use [SPARK-41416][SQL] ... and link the existing JIRA, since it describes the same rewrite and motivating Q95 case. Test fixture cleanup Agreed. I'll switch the suite to withTable / withTempView so the fixtures are cleaned up even when a test fails. Leaf / expression allowlists Thanks for checking this. The current Parquet-only boundary is more conservative than principled: Parquet is the only file source I validated while developing the rewrite, so I kept the leaf allowlist limited to what had actually been tested. I don't currently have a semantic reason to claim that ORC or JSON are less repeatable than Parquet here. Your measurement makes that distinction pretty clear. I'll revisit the leaf check and separate the actual repeatability requirement from the storage format, rather than treating Parquet itself as the property we depend on. Alias.explicitMetadata Agreed. canonicalizeWrapper should preserve it consistently with the sibling helper. I'll fix that. I'll push these changes together with the remaining items from the earlier review threads. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
