diff --git a/.ai/skills/review-comet-expression-pr/SKILL.md b/.ai/skills/review-comet-expression-pr/SKILL.md index b7690a19271..2c83d9d15ba 100644 --- a/.ai/skills/review-comet-expression-pr/SKILL.md +++ b/.ai/skills/review-comet-expression-pr/SKILL.md @@ -74,6 +74,9 @@ Location: `spark/src/main/scala/org/apache/comet/serde/` - [ ] `getSupportLevel` reflects true compatibility rather than the happy path - [ ] Serde lives in the appropriate file (`datetime.scala`, `strings.scala`, `arithmetic.scala`, and so on) - [ ] ANSI and `fail_on_error` handling matches the constraints in `adding_a_new_expression.md` +- [ ] A change that routes a whole class of expressions to the JVM codegen dispatcher lists the new + shapes it admits and tests one of each, including decimal results whose scale differs from the + declared type and boolean inputs from sliced batches (#6424, #6425) ### Registration in `QueryPlanSerde.scala` @@ -103,6 +106,12 @@ Location: `native/spark-expr/src/`, registered in `comet_scalar_funcs.rs`. - [ ] No panics. Use `Result`. - [ ] Batch operations rather than row-by-row where a kernel exists - [ ] Invalid UTF-8 going into a native `StringType` goes through `decode_utf8_spark_lossy` +- [ ] An aggregate merges partial states in Spark's order of operations, not just with Spark's + buffer layout. A different floating-point order fails Spark's exact checks, such as `m2 == 0`, + when a constant is merged from several partitions (#6423). +- [ ] Comparisons, grouping and set operations treat `-0.0` and `0.0`, and NaN payloads, as Spark + does, including inside arrays and structs. Where a DataFusion kernel treats them differently, + the serde falls back (#5507, #5701). Before accepting a hand-written kernel, ask whether the function already exists upstream in DataFusion or the `datafusion-spark` crate. Comet prefers wiring an upstream function over carrying @@ -163,6 +172,11 @@ single file by appending a substring of its name to the suite argument. - [ ] Timezone handling tested for timestamp and datetime expressions, including a non-UTC session timezone and timestamps with and without timezone - [ ] SQL syntax gated with `MinSparkVersion` when it only parses on newer Spark +- [ ] For a function from another project, such as an Iceberg transform, the expected values come + from that project's Java implementation, which is what Spark runs, not from iceberg-rust + (#6426) +- [ ] A `query ignore(...)` for a result that differs from Spark comes with a fallback for that + case, so the divergence doesn't ship natively by default (#5701) - [ ] `expect_error` patterns substring-match what both Spark and Comet actually throw - [ ] One expression per SQL file - [ ] Comet Scala literal tests disable constant folding: diff --git a/.ai/skills/review-comet-memory-pr/SKILL.md b/.ai/skills/review-comet-memory-pr/SKILL.md index fbd4610b20a..cad71116c7f 100644 --- a/.ai/skills/review-comet-memory-pr/SKILL.md +++ b/.ai/skills/review-comet-memory-pr/SKILL.md @@ -182,6 +182,10 @@ what test was added. Ask for at least one of: - A trace comparing `jemalloc_allocated` against the summed `thread_NNN_comet_memory_reserved` values, which is the only way to see the accounting gap - Spill counts from `spark.comet.explain.native.enabled=true`, before and after +- For a DataFusion upgrade, or a change to a spilling operator, a run where the operator spills + under a tight pool and then reads its spill back. The DataFusion 55.1 final aggregate lost the + ability to spill again during that replay, and only a run at a small off-heap size showed it + (#6254). - For a fix to an OOM report, the exit code that identifies which budget was exceeded. 137 or `OOMKilled` is the cgroup. 52 with `java.lang.OutOfMemoryError` is JVM heap. A failed task with `SparkOutOfMemoryError` and a surviving executor is Spark's pool, the only one of the three that diff --git a/.ai/skills/review-comet-pr/SKILL.md b/.ai/skills/review-comet-pr/SKILL.md index 4d6d8558612..050a60802ce 100644 --- a/.ai/skills/review-comet-pr/SKILL.md +++ b/.ai/skills/review-comet-pr/SKILL.md @@ -97,6 +97,73 @@ A PR that quietly marks something `Compatible` while the diff shows a known dive most important thing to catch. Keep reasons concise and link a tracking issue when the behavior is known to differ. +Two disguised forms of the same thing: + +- A divergence documented in the compatibility guide on a path that is enabled by default. If the + guide says the results differ, the default has to fall back for that case (#5469, #5507). +- A test switched to `ignore` in the PR because Comet's answer now differs from Spark's. An ignored + test is a known divergence, so the code needs a fallback for that case (#5701). + +### Behavior change against the latest release + +Users upgrade from a release, not from `main`. So the question is not only what the PR changes +relative to `main`, but whether a user moving from the latest release to a build with this PR sees +different behavior. `main` may already carry unreleased changes in the same code, and a PR that +looks harmless against `main` can finish turning a released behavior into a different one. + +Find the latest release branch, the highest `branch-X.Y`: + +```shell +git ls-remote --heads https://github.com/apache/datafusion-comet 'branch-*' | sort -t- -k2 -V | tail -1 +``` + +Fetch the PR head and diff the files the PR touches against that branch, not against `main`: + +```shell +git fetch https://github.com/apache/datafusion-comet branch-X.Y:release-X.Y pull//head:pr- +git diff release-X.Y pr- -- +``` + +Then ask, for each code path the PR touches, whether any of these differ from the release: + +- the result for some input, including null, NaN, `-0.0`, empty, overflow, and timezone cases +- whether a query raises an error, and which error +- whether an operator or expression runs natively or falls back to Spark +- a config default, a config name, or a support level +- performance or memory use on an existing path + +The tests the PR adds are a good probe. If a new test would fail on the release branch, the PR +changes released behavior, and you should know which of those two cases it is. Running the test +against the release branch is the cheapest way to find out when the answer is not obvious from +the code. + +Every behavior change against the release is one of two things: + +- **Intended.** A bug fix that makes Comet match Spark, or a deliberate change. The PR description + should say so, user-facing changes need a note in the user guide or compatibility docs, and a + correctness fix should be considered for backport to the release branches per + `docs/source/contributor-guide/backporting.md`. +- **Unintended.** The PR, alone or together with unreleased changes already on `main`, makes Comet + diverge from Spark where the release did not, or makes a released path slower. This is a + correctness or performance regression, and it falls under the request-changes rule below. + +Look hardest when the PR is one of the three kinds of change behind most of the regressions that the +1.1.0 audit found ([#6399](https://github.com/apache/datafusion-comet/issues/6399)): + +- **A path that becomes native by default.** The query used to fall back to Spark and was right. + Find the inputs where the native path differs from Spark, and either test them or fall back for + them. Making map and struct literals native exposed a type mismatch in the native `IF` (#6334). + Constant metadata columns went native with a per-split value that DataFusion and Spark assign + differently (#6505). +- **A removed fallback or guard.** List everything the fallback was shielding, not just the case the + PR is about. Removing the Iceberg complex-type null-check fallback also exposed every `explode` of + an Iceberg array to a schema-evolution bug in iceberg-rust (#6504). +- **A broad routing change.** A catch-all that sends more expressions down a path, such as the JVM + codegen dispatcher, admits shapes the path never handled. List the new shapes and test one of each + (#6424, #6425). + +Report the comparison in the review even when nothing changed, so the reviewer knows it was done. + ### Spark version coverage Comet supports several Spark versions. Version-specific behavior belongs in the shims under @@ -104,6 +171,11 @@ Comet supports several Spark versions. Version-specific behavior belongs in the version string in shared code, and not in native Rust. If the PR adds a shim for one 4.x version, check that the sibling 4.x source sets got it too. +When Spark changed the behavior in a patch release, such as SPARK-55969 or SPARK-54918, a check on +the minor version is wrong for every earlier patch. CI builds only the newest patch of each line, so +it can't catch that (#6042, #5701). The pull request CI also runs only the default Spark profile, so +logic that depends on the Spark version needs the matching `run-spark-*` labels (#6156). + ### Configuration New configs go in `CometConf.scala` and must follow @@ -111,20 +183,56 @@ New configs go in `CometConf.scala` and must follow and that the description reads as user-facing documentation, because it is. `configs.md` is generated from it. +A new behavior that is on by default and trades performance for some workloads needs a supported +config, not a testing one, to turn it off before merge (#6466). + ### Tests - Does the PR test the thing it changed, or only that nothing else broke? - Are the tests in the right framework for the area? The area skills say which. - Does a bug fix come with a test that fails without the fix? +- Do the tests compare against Spark? A comparison with Comet's own accumulator, or with a second + implementation such as iceberg-rust instead of the Iceberg Java that Spark runs, can agree on the + wrong answer (#6423, #6426). - New suites must be registered in both `.github/workflows/pr_build_linux.yml` and `pr_build_macos.yml`. +For a change on a default path, look for tests with the inputs that broke earlier changes: + +- `-0.0` next to `0.0`, and NaN with different payloads, including inside arrays and structs (#5469, + #5507, #5701) +- Values at a type or unit boundary, including negative timestamps before 1970 (#6426) +- A constant that binary floating point can't represent, such as 0.1, aggregated across partitions + (#6423) +- A batch where every row takes the same branch (#6334) +- Output that the native side slices, so it reaches the JVM past the first batch with a non-zero + offset (#6464) +- A Parquet file that Spark splits into several partitions, an empty file, and an Iceberg file + written before a nested field was added (#6504, #6505, #6506) +- Data that defeats a heuristic, such as distinct keys at the start of a task followed by repeats + (#6466) +- One of a pair of aliases configured without the other (#5825) + ### CI `gh pr checks --repo apache/datafusion-comet`, and again with `--failed` for detail. Summarize failures in the review. Do not compare against failures on `main`. +Some paths have no CI coverage at all. Nothing in CI reads from a real object store, runs Iceberg's +forward-compatibility tables, or makes a spilled operator replay under a tight memory pool. A change +on one of those paths needs a local run, and the review should ask for one (#5759, #6254). + +### Dependency upgrades + +Review a dependency bump as a set of behavior changes, not an API migration. For DataFusion, arrow, +parquet and iceberg-rust, diff the upstream source of what Comet calls on default paths. Both +versions are in the local Cargo registry after a build of each side. A test that the bump changes to +`ignore` or to a new expected value marks a behavior change that needs a fallback or an explicit +decision. The DataFusion 55 upgrade changed signed-zero handling in `array_distinct` and +`array_union` (#5701), the spill replay of the final aggregate (#6254), and, through iceberg-rust, +task validation (#5759). + ## 6. Documentation Freshness Every PR carries a documentation question, and it has two halves. @@ -192,13 +300,16 @@ Present your review as guidance for the reviewer: 1. **PR Summary**, brief description of what the PR does 2. **Areas**, which sibling skills you loaded and why 3. **CI Status**, summary of CI check results -4. **Findings**, organized by area -5. **Suggested Review Comments**, specific comments the reviewer could leave, with file and line +4. **Behavior vs Release**, which release branch you compared against, and every behavior change + you found, each marked intended or unintended. Say "no change" when there is none. +5. **Findings**, organized by area +6. **Suggested Review Comments**, specific comments the reviewer could leave, with file and line references. Everything here is something you expect the author to address. Anything that did not clear the bar above should not appear. -6. **Review State**, how to submit the review. Use the first case that applies: +7. **Review State**, how to submit the review. Use the first case that applies: - **Request changes** when any finding is a correctness problem that the PR introduces or a - performance regression. Name those findings. + performance regression, including an unintended behavior change against the latest release. + Name those findings. - **Approve**, or dismiss the earlier review, when this is a re-review and the findings behind the reviewer's earlier **Request changes** review have all been addressed. - **Comment** otherwise.