review-comet-expression-pr

<!-- 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. --> Expression-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 what is specific to expressions. ## Read the Contributor Guide First | Doc | What you need from it | | ---------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- | | `docs/source/contributor-guide/adding_a_new_expression.md` | The serde contract, support levels, when to set the return type explicitly, shimming | | `docs/source/contributor-guide/sql-file-tests.md` | The test framework expression PRs are expected to use, and every directive it supports | | `docs/source/contributor-guide/optimizing_expressions.md` | The benchmark workflow and the no-regression rule, for PRs that change an existing kernel | | `docs/source/contributor-guide/timezones.md` | The timestamp label rule, where the session timezone comes from, and how to test it, for datetime expressions, casts, and anything that returns a timestamp | Hold the diff against these. If the PR does something a guide says to do differently, either the PR is wrong or the guide is out of date, and you need to say which. ## 1. Read the Spark Source **For any PR that adds or modifies an expression, you must read the Spark source to understand the canonical behavior.** This is the authoritative reference for what Comet must match. Do not review an expression PR from the diff alone. Clone Spark into `/tmp/spark` if it is not already there, using a shallow clone, then find the expression class under `sql/catalyst/src/main/scala` by searching for its `case class` declaration. Read the Spark implementation carefully. Pay attention to: - `eval` and `doGenCode` / `nullSafeEval`. These define the exact behavior. - `inputTypes` and `dataType`. These define which types Spark accepts and what it returns. - Null handling. Does it use `nullable = true`? Does `nullSafeEval` handle nulls implicitly? - Special cases, guards, and `require` assertions. - ANSI mode branches, usually `SQLConf.get.ansiEnabled` or `failOnError`. - Behavior differences across the Spark versions Comet supports. If the expression changed between 3.5 and 4.0, the serde needs a shim, not a single implementation. Then read the Spark tests for the expression, under the test source trees in `sql/`. Compare Spark's behavior against the PR and identify edge cases Spark tests that the PR does not, types Spark supports that the PR does not handle, and behavioral differences that should be marked `Incompatible`. ## 2. Scala Serde Location: `spark/src/main/scala/org/apache/comet/serde/` - [ ] Expression class correctly identified - [ ] All child expressions converted via `exprToProtoInternal` - [ ] Return type correctly serialized - [ ] `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) - [ ] A timezone-aware expression serializes `CometTimeZone.nativeId(expr.timeZoneId)`, the timezone Spark stamped on it in a form native code can parse, rather than `SQLConf.get.sessionLocalTimeZone` or the JVM default, and returns `CometTimeZone.supportLevel` from `getSupportLevel` when `nativeId` gives `None` ### Registration in `QueryPlanSerde.scala` - [ ] Added to the correct map (temporal, string, arithmetic, and so on) - [ ] No duplicate registrations - [ ] Import statement added ### Return type collisions with DataFusion built-ins If the PR registers a Spark function whose name is also defined by `datafusion-functions`, such as `levenshtein`, `concat`, `coalesce`, `sha2`, or `regexp_replace`, check that the serde sets the return type explicitly via `scalarFunctionExprToProtoWithReturnType` rather than `scalarFunctionExprToProto` or the bare `CometScalarFunction(name)` shortcut. Without an explicit return type the native planner consults DataFusion's UDF registry first for type resolution, and any arity or input-type difference between the Spark and DataFusion versions fails native execution with `Error from DataFusion: Function 'X' expects N arguments but received M`. The Comet UDF is only swapped in after DataFusion's signature validation passes. See "When to set the return type explicitly" in `adding_a_new_expression.md`. ## 3. Rust Implementation Location: `native/spark-expr/src/`, registered in `comet_scalar_funcs.rs`. - [ ] Matches DataFusion and Arrow conventions - [ ] Null handling is correct, including nulls inside nested types - [ ] 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). - [ ] A `TimestampType` result is labelled `"UTC"`, in `return_type()` or `data_type()` and in the arrays the kernel builds. Not the session timezone, and not `None`. - [ ] Local-time work uses the session timezone from the proto. DataFusion's own datetime functions take the timezone from the input's label, so wiring one in for a timezone-aware Spark expression evaluates it in UTC unless the session timezone is passed explicitly. - [ ] A UTC fast path matches a fixed list of UTC aliases, as `is_utc_timezone` in `extract_date_part.rs` does, and sends every other timezone ID down the general path 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 its own copy. ## 4. Tests **Expression tests should use Comet SQL Tests (`CometSqlFileTestSuite`) where possible.** The framework runs each query through both Spark and Comet and compares results, with no Scala code. Fall back to Comet Scala Tests in `CometExpressionSuite` only when the SQL framework cannot express the test, for example complex `DataFrame` setup or programmatic data generation. Test location: `spark/src/test/resources/sql-tests/expressions/<category>/`, with categories `aggregate/`, `array/`, `string/`, `math/`, `struct/`, `map/`, `datetime/`, `hash/`, and others. ```sql statement CREATE TABLE test_crc32(col string, a int, b float) USING parquet statement INSERT INTO test_crc32 VALUES ('Spark', 10, 1.5), (NULL, NULL, NULL), ('', 0, 0.0) -- default mode: verifies native Comet execution and that the result matches Spark query SELECT crc32(col) FROM test_crc32 -- compares results without requiring native execution query spark_answer_only SELECT crc32(cast(a as string)) FROM test_crc32 -- allows numeric variance for floating-point results query tolerance=0.0001 SELECT cos(v) FROM test_trig -- asserts fallback to Spark occurs query expect_fallback(unsupported expression) SELECT unsupported_func(v) FROM test_table -- verifies both engines throw matching exceptions query expect_error(ARITHMETIC_OVERFLOW) SELECT 2147483647 + 1 -- skip a query with a known bug, with the issue link query ignore(https://github.com/apache/datafusion-comet/issues/NNNN) SELECT known_buggy_expr(v) FROM test_table ``` Run the whole suite with `-Dsuites="org.apache.comet.CometSqlFileTestSuite" -Dtest=none`, or a single file by appending a substring of its name to the suite argument. **Verify all test requirements, whichever framework is used:** - [ ] Basic functionality tested against column data, not only literals - [ ] Null handling tested - [ ] Edge cases tested: empty input, overflow, boundary values, negative values - [ ] Both literal and column arguments tested, in every combination for multi-argument expressions. They take different code paths. - [ ] Timestamp and datetime expressions are tested as "Testing timezone-sensitive code" in `timezones.md` describes. That means several session timezones through `ConfigMatrix`, including `Etc/UTC` and a zone with DST, both `TIMESTAMP` and `TIMESTAMP_NTZ` inputs, and a result that is compared or fed to another expression rather than only projected. - [ ] 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: ```scala withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> "org.apache.spark.sql.catalyst.optimizer.ConstantFolding") { checkSparkAnswerAndOperator("SELECT func(literal)") } ``` ## 5. Performance **For PRs that add a new native expression, performance is not optional.** The point of a native implementation is to be faster than Spark's codegen. If it is not faster, it may not be worth carrying. 1. Does the PR description contain benchmark numbers comparing Comet against Spark? If not, ask for them. 2. Does the PR add a benchmark under `spark/src/test/scala/org/apache/spark/sql/benchmark/`? 3. Are the benchmarks representative, with realistic data sizes and the data shapes that break optimizations, such as nulls, dictionaries, and wide strings? 4. Does the Rust implementation allocate or copy more than it needs to, process row by row where a kernel exists, redo type conversions, or revalidate UTF-8 repeatedly? 5. If a benchmark shows Comet slower than Spark, flag it. The PR needs to explain why that is acceptable or include a plan to fix it. For PRs optimizing an existing kernel, `optimizing_expressions.md` defines the no-regression rule and the requirement to record a performance audit. Hold the PR to both. ## 6. Documentation ### Generated, review the source instead The compatibility pages under `docs/source/user-guide/latest/compatibility/expressions/` and `docs/source/user-guide/latest/configs.md` are produced by `GenerateDocs` and regenerated by CI on merge. Never ask the contributor to hand-edit them. Check what feeds them: - Expression marked `Incompatible(Some("..."))` but `getIncompatibleReasons()` is empty, so the compat page shows it as supported with no caveats. - `Unsupported(Some("..."))` for specific types or argument shapes with no `getUnsupportedReasons()` to surface the limitation. - Reason strings that drift from the `notes` passed to `Compatible` / `Incompatible` / `Unsupported`. They need not match exactly, but consistency helps. - Reason strings too terse to be useful in user-facing docs: a single word, no context, no link to a tracking issue when behavior is known to differ. ### Hand-edited, the PR should update these - `docs/source/user-guide/latest/expressions.md`, the supported-expressions list. A new expression belongs here. - Other `latest/compatibility/` pages such as `floating-point.md`, `regex.md`, `operators.md`. ### Does the PR make the contributor guide stale? Check `adding_a_new_expression.md` against the diff. It goes stale when a PR: - Changes the `CometExpressionSerde` interface, the support-level API, or the `getIncompatibleReasons` / `getUnsupportedReasons` contract - Adds or renames an expression map in `QueryPlanSerde.scala`, since the guide names them - Changes how the return type is resolved, which would invalidate the "when to set the return type explicitly" section - Changes the shimming pattern or adds a Spark version to the set the guide lists - Changes how scalar functions are registered in `comet_scalar_funcs.rs` Check `sql-file-tests.md` when the PR adds, renames, or changes a test directive. The directive reference in that doc is the only place they are documented. ## Common Expression Review Findings 1. **Incomplete type support**, Spark supports types the PR does not handle 2. **Missing edge cases**, null, overflow, empty string, negative values 3. **Wrong return type**, it must match Spark exactly 4. **Tests in the wrong framework**, Scala tests where a SQL file test would do 5. **Missing `getSupportLevel`**, divergences left undeclared rather than marked `Incompatible` 6. **Version-specific Spark behavior implemented once**, with no shim or version check, or with a protobuf field, native argument, or native function named for the Spark version (`spark_420_plus`) instead of the behavior (`wrap_second_millisecond_overflow`) 7. **Name collides with a DataFusion built-in** and no explicit return type 8. **Timestamp result mislabelled**, with the session timezone or no timezone instead of `"UTC"`. It only shows once the result is compared or fed to another expression.