hhr293 commented on PR #58424: URL: https://github.com/apache/spark/pull/58424#issuecomment-5644528432
> **I would hold approval until the existing timestamp-cast issue is fixed.** I completed five independent review scopes against `15aa88d1`; the code is unchanged, and I found no additional confirmed correctness defect. > > **[P2] The repeatability check still accepts clock-dependent casts.** Converting a time-only string such as `"12:03:17"` to a timestamp supplies today’s date. If the two self-join evaluations cross midnight, they can produce different values; replacing them with one aggregation changes membership results. The [existing review comment](https://github.com/apache/spark/pull/58424#discussion_r3963602271) remains applicable. > > This review adds a concrete requirement for the fix: **checking only direct string-to-timestamp casts is insufficient.** For example: > > ```sql > CAST(CAST(ss AS ARRAY<TIMESTAMP>) AS STRING) > ``` > > With `ss` containing time-only strings, the conversion inside the array has the same date dependency. My new probe confirmed that the optimization fires and this nested conversion survives. Rejecting complex _join keys_ does not exclude this case because the final key is a string. > > The rest of the implementation held up under review: > > * The existence-only rewrite, NULL handling, tuple membership, and preservation of surrounding joins appear sound. > * Relation identity, output-column mapping, optimizer placement, and configuration controls revealed no additional defect. > * Tests would be stronger if result comparisons preserved duplicate counts instead of using `.toSet`, and ANSI tests asserted expected results or errors. > * Fixture cleanup would improve isolation between tests; the superclass already resets the catalog after the suite, so I would not report a cross-suite leak. > > **Validation:** CI artifacts show all 31 tests passing. Today’s four additional probe cases passed. The earlier controlled-clock reproduction and today’s probe use a partial Spark 4.0.2 overlay; I did not complete a clean native build of this PR or reproduce nested-cast divergence across midnight. > > The optimization remains worth pursuing: it removes potentially quadratic pair comparisons for queries that only need an existence answer. Its narrow scope and maintenance cost are reasonable tradeoffs for an opt-in first version, provided its repeatability checks are sound. Thanks for the detailed review. I’ve fixed the timestamp-cast issue, including the nested cast case, and pushed the corresponding tests in the latest commits. Could you please take another look? -- 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]
