pingzh commented on PR #5785: URL: https://github.com/apache/datafusion-comet/pull/5785#issuecomment-5641977851
@andygrove The five concrete requests in [your review](https://github.com/apache/datafusion-comet/pull/5785#pullrequestreview-5171034064) are addressed in the current head, `df93277a84de4b1c6cc190a487357ca207c7db0b`: | Request | What changed | | --- | --- | | Reversion gives wrong answers | Unwrap `CometLocalTopKExec` before generic Spark restoration. Added real-Parquet regressions for offsets, projections that omit the sort key, and AQE/filtering off/on. | | Redundant single-partition heap | When the local TopK has a matching limit and sort order and the input has one partition, the final stage applies only limit/offset/projection. Tests inspect the actual serialized-plan builder. Matched base/fused benchmarks are in the PR description. | | M1: independent fusion switch | Added `spark.comet.exec.topK.fusion.enabled`, default `true`. Disabling it retains the ordinary native TopK path. | | M2: shared timestamp protection | Moved the recursive projected-timestamp guard into the reader helper used by both TopK and joins. Tests cover projected timestamps, unprojected timestamps, and the join caller. | | Integer-key pruning coverage | Parameterized Spark tests across TINYINT, SMALLINT, INT, and BIGINT, including Parquet's narrow-integer annotations and page-index/row-filter settings. | The new reversion, fusion-switch, and integer-key tests pass in [current Spark 4.1 CI](https://github.com/apache/datafusion-comet/actions/runs/34650801192/job/103434281328), and the [Rust job passes](https://github.com/apache/datafusion-comet/actions/runs/34650801192/job/103433209206). Fusion eligibility still requires one direct signed integer key. The separate pre-existing timestamp-cast discrepancy involving inferred `IS NOT NULL` remains and is explicitly documented. The wider benchmark also found a remaining fusion slowdown on wide descending data; that limitation is documented, with further pipeline optimization deferred. The reported benchmark measurements predate the latest DataFusion 55.1 rebase and remain labeled with their actual revisions and dependencies. Could you take another look at the current head? -- 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]
