sunchao commented on code in PR #6018: URL: https://github.com/apache/datafusion-comet/pull/6018#discussion_r4050482813
########## .ai/skills/review-comet-shuffle-pr/SKILL.md: ########## @@ -0,0 +1,219 @@ +--- +name: review-comet-shuffle-pr +description: Use when reviewing a DataFusion Comet pull request that touches native or JVM columnar shuffle, the shuffle writers and readers, partitioning, the Arrow IPC block format, shuffle compression, or the Celeborn integration. Load alongside review-comet-pr. +argument-hint: <pr-number> +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +Shuffle-specific review for Comet PR #$ARGUMENTS. + +**REQUIRED BACKGROUND:** Use `review-comet-pr` for PR metadata, existing comments, CI, the review +bar, and the output format. This skill only covers shuffle. + +## Read the Contributor Guide First + +| Doc | What you need from it | +| ---------------------------------------------------- | ---------------------------------------------------------------------- | +| `docs/source/contributor-guide/native_shuffle.md` | Selection rules, architecture, partitioning, block format, spilling | +| `docs/source/contributor-guide/jvm_shuffle.md` | Writer variants, handle selection, the row-based path, spill mechanics | +| `docs/source/contributor-guide/memory_management.md` | Where shuffle memory comes from, which differs between the two paths | + +**Read both shuffle docs even if the PR only touches one path.** The two implementations share the +manager, the dependency, the reader, and the on-disk format, and a change to one side of a shared +piece is the most common way to break the other. + +## 1. Which Implementation + +| Implementation | Selected when | +| ------------------------------------- | --------------------------------------------------------------------------------------------------------- | +| Native, `CometExchange` | `shuffle.mode` is `native` or `auto`, child is a `CometPlan`, supported partitioning, supported key types | +| JVM columnar, `CometColumnarExchange` | `shuffle.mode` is `jvm`, or the child is row-based, or a partition key type native shuffle cannot handle | + +Complex types are fully supported as **data** columns in both. The restriction is on **partition +keys**, and it is not the same rule for the two partitionings: + +- **`RangePartitioning` is primitive-only, unconditionally.** + `supportedRangePartitioningDataType` rejects every nested type, because native cannot sort them, + and rejects collated strings, because Comet compares raw bytes. It also rejects float and double + when `spark.comet.exec.strictFloatingPoint` is on. Review Comment: ### Correctness [P2] Align the strict-mode range-key rule with current main Could you update this restriction and the matching paragraph in `native_shuffle.md` for #5981? That change is already in the current CI merge `bbb5c572b`, whose main parent is `c56cef77a`. In that tree, [`supportedRangePartitioningDataType`](https://github.com/apache/datafusion-comet/blob/bbb5c572be59f839973e3286bf5cca36e0bebccb/spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/CometShuffleExchangeExec.scala#L535-L543) accepts scalar float and double keys even in strict mode, and [`CometSortOrder`](https://github.com/apache/datafusion-comet/blob/bbb5c572be59f839973e3286bf5cca36e0bebccb/spark/src/main/scala/org/apache/comet/serde/CometSortOrder.scala#L41-L54) marks them compatible. The [existing range-shuffle test](https://github.com/apache/datafusion-comet/blob/bbb5c572be59f839973e3286bf5cca36e0bebccb/spark/src/test/scala/org/apache/comet/exec/CometNativeShuffleSuite.scala#L1146-L1166) expects a native exchange for both strict settings with `SortOrder.allowIncompat ible=false`. The new rule would therefore tell reviewers to reject a supported native plan as soon as this PR lands on current main. The restriction still applies to nested types and collated strings, not scalar floating-point range keys. -- 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]
