zhangfengcdt commented on PR #6928: URL: https://github.com/apache/datafusion-comet/pull/6928#issuecomment-6196760082
> I reproduced the `COUNT(DISTINCT)` case sunchao reported with a real query, and it is still open. On this head, a one-partition parquet table with 22 doubles ending in `0.0` then `-0.0` (the values from sunchao's repro, plus a constant int `d`) and `SELECT cast(percentile(v, 0.0) as string), count(DISTINCT d)` returns `-0.0` from Comet and `0.0` from Spark. The same query passes when I force `signed_zero_keys` to false in the serde, so the mismatch comes from the new replay and not from the existing selection. The doc comment on `spark_iterates_negative_zero_first` already says the order can differ here, but `CometPercentile` still reports `Compatible()` for that plan shape. Could the serde skip the replay (set `signed_zero_keys` to false) or fall back when the aggregate is in a `PartialMerge` stage, so a query that matched Spark does not start returning the other zero? > > The early-emit chunking case and the sort-based fallback case that sunchao and comphead described have the same shape. Each is a path where the replay model does not match what Spark builds, and the replay moves a previously matching answer. Could the PR either model those paths or limit the replay to the shapes it is known to reproduce, and say in `signedZeroKeysNote` which shapes remain unmatched? I would also like replies on comphead's questions about `partial_ends` being shuffled for non-float inputs and for Spark before 3.5.2, since that extra state ships on every query that uses `percentile` today. > > For the tests, I ran `percentile_zero_tie.sql` and `percentile_partials.sql` through `CometSqlFileTestSuite` on this head and both pass. With the serde forced to `setSignedZeroKeys(false)` both fail, so they do pin the new behavior. The `datafusion-comet-spark-expr` percentile unit tests also pass. None of them cover a `PartialMerge` plan, so a case with `percentile` next to `count(DISTINCT ...)` would be worth adding once the behavior is decided. Thank for confirming the two issues, and I think they both should have been fixed now with the few new commits. The replay now maps to `PartialMerge` stages, so the `count(DISTINCT)` case matches Spark and is covered by a new SQL fixture and unit test. The partial boundaries are only tracked for `FLOAT`/`DOUBLE` on Spark 3.5.2+; and the sort-based fallback and early-emit paths, which cannot be detected at plan time, are named in the note attached to the `Compatible` result. comphead's threads are answered inline, and the branch is merged with main. -- 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]
