andygrove opened a new issue, #6634:
URL: https://github.com/apache/datafusion-comet/issues/6634
Part of #6615.
## What / Why
This issue moves hash and bitwise coverage to Comet SQL tests under
`sql-tests/expressions/hash/` and `sql-tests/expressions/bitwise/`. The sources
are `CometHashExpressionSuite`, `CometBitwiseExpressionSuite`, and the hash and
bitwise tests in `CometExpressionSuite`. Static analysis on `main` at
3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometExpressionSuite` | 3 | 4 | 0 | 0 | 0 | 7 |
| `CometHashExpressionSuite` | 6 | 35 | 0 | 0 | 1 | 42 |
| `CometBitwiseExpressionSuite` | 1 | 6 | 1 | 0 | 0 | 8 |
| **Total** | **10** | **45** | **1** | **0** | **1** | **57** |
`hash.sql` hashes only values built inside the query. The Scala tests hash
lists, structs and maps read from Parquet, cover every primitive type at its
boundaries, and include the #5754 required-child and #5778 batched
array-of-struct regressions. The bitwise fixture runs `bit_get` only as
`spark_answer_only` and has no tinyint, smallint or boolean `bit_count`.
## Work
Line numbers refer to `CometHashExpressionSuite` unless marked.
**Hash**
- `hash/hash_types.sql` (new): L38, L46, L54, L62, L70, L103, L111, L120.
- One column per type: boolean, tinyint, smallint, int, bigint, binary,
date and timestamp.
- Each column holds 0, -1, the type's min and max, and NULL
(true/false/NULL for boolean).
- Query: `SELECT c, hash(c), xxhash64(c)`.
- Also add timestamp_ntz. Only the fuzz test hashes that type today.
- `hash/hash_decimal.sql` (new): L132, L142, L153, L162, L171, plus
`CometExpressionSuite` L2751 and L2772.
- Plain queries for decimal(1,0), (10,2), (17,2), (18,0), (18,2) and
(18,10), each with 1.23, -1.23, 0.0 and NULL.
- `expect_fallback(precision > 18)` for decimal(19,2), (20,2), (38,10) and
(38,37), and for `array<decimal(20,2)>`, `struct<a int, b decimal(20,2)>` and
`map<string, decimal(20,2)>`.
- The map case needs `spark.sql.legacy.allowHashOnMapType=true`, which is
file-level, so it may need its own file.
- `expect_fallback` is stronger than the Scala answer-only check: the hash
serdes declare precision > 18 Unsupported and have no dispatcher route.
- `hash/hash_nested.sql` (new, with `Config:
spark.sql.legacy.allowHashOnMapType=true`): L184-L250, L356-L395, L423-L487,
L517, L533, L557.
- Lists, structs and maps read from Parquet, including empty, NULL,
null-element and null-field rows.
- Multi-column chains (int, array, string, struct), and an all-NULL
single-row file.
- A 1000-element list via `INSERT INTO t SELECT sequence(1, 1000)`.
- The 9-row oracle for the batched `array<struct>` path (#5778), and the
nested case that can't be batched.
- `hash/hash_required_child.sql` (new): L262, L288, L313.
- Build the table with CTAS, for example `CREATE TABLE t USING parquet AS
SELECT /*+ COALESCE(1) */ c FROM VALUES (named_struct('a', 1)), (NULL),
(named_struct('a', 3)), (NULL) AS v(c)`. The inline table's column type stays
`struct<a: int NOT NULL>`, and CTAS writes that schema as-is.
- Do not use CREATE + INSERT. On Spark 3.5+,
`TableOutputResolver.resolveStructType` rebuilds struct columns from
`GetStructField`, so the child is written optional and the fixture would pass
without testing anything.
- The framework cannot assert the written schema, so check once, for
example by reverting #5754, that the fixture fails without the fix.
- `hash/hash.sql` (`CometExpressionSuite` L2713)
- Int Min/Max, and float Max, Min and MinPositive.
- Strings containing `\t`, `\r`, `\n`, `/`, `:` and `T`.
**Bitwise**
- `bitwise/bitwise.sql` (existing)
- `CometBitwiseExpressionSuite` L57: bigint `shiftright` / `shiftleft`
with literal and column amounts. The serde casts the amount to bigint, a path
no fixture reaches.
- L91, L112: a typed table (tinyint, smallint, int, bigint).
- Values: NULL, 0, ±1, each type's min and max,
4294967295/4294967294/4294967293, and wrapped byte/short values.
- Query `bit_get` at boundary positions: 0 and 7 for tinyint, 0 and 15
for smallint, 0 and 31 for int, and 0, 31, 32 and 63 for bigint. The Scala
positions come from unseeded `Random`.
- The Scala tests assert native `bit_get`, so also try upgrading
`bitwise.sql:111` and `:121` from `spark_answer_only` to `query`.
- L139, L162, L184: `bit_count` over tinyint, smallint, int, bigint and
boolean with min, max, -1, 0 and NULL. Also `bit_count(true)` and
`bit_count(false)`, and the bigints next to 2^32.
- `bitwise/deep_bitwise_chains.sql` (new, `CometExpressionSuite` L3863):
200-term `&`, `|` and `^` chains. These stay left-deep in SQL.
**Blocked on #6617: `CometBitwiseExpressionSuite` L77** ("bitwise_get -
throws exceptions")
- As written, the test never reaches Comet. ConstantFolding throws during
optimization in both runs, and the two `bit_count(x, y)` calls are analyzer
arity errors. Drop those two.
- A column-based fixture would hit datafusion-spark's message, "bit_get:
position -30 is out of bounds. Expected pos < 32 and pos >= 0". That matches
neither Spark 3.x ("Invalid bit position: -30 is less than zero") nor 4.x
(`INVALID_PARAMETER_VALUE.BIT_POSITION_RANGE`).
- This was found by reading the code, not by running it. If a run confirms
it, file it as an error-parity bug while converting.
## Stays in Scala
- **L577, the hash fuzz test.** Its type breadth is covered by
`hash_types.sql` once timestamp_ntz is added. The randomness is the point.
## Notes on the Scala tests
- **"hash - float" and "hash - double" insert a bare `-0.0`,** which is
stored as +0.0. `hash.sql` uses `float('-0.0')` and `double('-0.0')`, so the
fixture is the stronger test.
- **Constant-folded literals.** `bit_count(true)`, `bit_count(false)` and
`~(11)` are folded by Spark in the Scala suite, but reach native code in a
fixture.
- **Near-duplicates.**
- "hash - empty strings and arrays" nearly duplicates "hash - multiple
columns with arrays".
- "bitwise_count - random values (spark gen)" is subsumed by the min/max
conversion.
## Done when
- Every test in the table below is accounted for. Either a fixture covers it
(the same inputs, configs and assertion strength, or stronger), the fixture
that already covers it is cited, or it stays in Scala for the reason given
above.
- Each converted Scala test is deleted in the PR that adds its coverage. A
suite left empty is deleted and removed from the suite lists in
`.github/workflows/pr_build_linux.yml` and `pr_build_macos.yml`.
- The new and changed fixtures pass on every Spark profile. Apply
`run-all-spark-profiles`, because pull requests run only the default profile.
<details><summary>Every Scala test in scope, with its verdict and target
fixture</summary>
| Suite | Line | Test | Verdict | Target |
|---|---|---|---|---|
| `CometBitwiseExpressionSuite` | 32 | bitwise expressions | covered |
bitwise/bitwise.sql |
| `CometBitwiseExpressionSuite` | 57 | bitwise shift with different
left/right types | convert | bitwise/bitwise.sql |
| `CometBitwiseExpressionSuite` | 77 | bitwise_get - throws exceptions |
blocked: #6617 | bitwise/bit_get_errors.sql (new; split MaxSparkVersion 3.5 /
MinSparkVersion 4.0 if patterns differ) |
| `CometBitwiseExpressionSuite` | 91 | bitwise_get - random values (spark
parquet gen) | convert | bitwise/bitwise.sql |
| `CometBitwiseExpressionSuite` | 112 | bitwise_get - random values (native
parquet gen) | convert | bitwise/bitwise.sql |
| `CometBitwiseExpressionSuite` | 139 | bitwise_count - min/max values |
convert | bitwise/bitwise.sql |
| `CometBitwiseExpressionSuite` | 162 | bitwise_count - random values (spark
gen) | convert | bitwise/bitwise.sql |
| `CometBitwiseExpressionSuite` | 184 | bitwise_count - random values
(native parquet gen) | convert | bitwise/bitwise.sql |
| `CometExpressionSuite` | 2035 | md5 | covered | hash/hash.sql |
| `CometExpressionSuite` | 2592 | hash functions | covered | hash/hash.sql +
hash/crc32.sql |
| `CometExpressionSuite` | 2618 | sha2 with all-literal arguments and
ConstantFolding disabled | covered | hash/hash.sql |
| `CometExpressionSuite` | 2713 | hash functions with random input | convert
| hash/hash.sql |
| `CometExpressionSuite` | 2751 | hash function with decimal input | convert
| hash/hash_decimal.sql (new) |
| `CometExpressionSuite` | 2772 | xxhash64 function with decimal input |
convert | hash/hash_decimal.sql (new) |
| `CometExpressionSuite` | 3863 | deep bitwise And/Or/Xor chains do not
overflow the protobuf recursion limit | convert |
bitwise/deep_bitwise_chains.sql (new) |
| `CometHashExpressionSuite` | 38 | hash - boolean | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 46 | hash - byte/tinyint | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 54 | hash - short/smallint | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 62 | hash - int | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 70 | hash - long/bigint | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 79 | hash - float | covered | hash/hash.sql |
| `CometHashExpressionSuite` | 87 | hash - double | covered | hash/hash.sql |
| `CometHashExpressionSuite` | 95 | hash - string | covered | hash/hash.sql |
| `CometHashExpressionSuite` | 103 | hash - binary | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 111 | hash - date | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 120 | hash - timestamp | convert |
hash/hash_types.sql (new) |
| `CometHashExpressionSuite` | 132 | hash - decimal (precision <= 18) |
convert | hash/hash_decimal.sql (new) |
| `CometHashExpressionSuite` | 142 | hash - decimal (precision > 18) |
convert | hash/hash_decimal.sql (new) |
| `CometHashExpressionSuite` | 153 | hash - array of decimal (precision >
18) falls back to Spark | convert | hash/hash_decimal.sql (new) |
| `CometHashExpressionSuite` | 162 | hash - struct with decimal (precision >
18) falls back to Spark | convert | hash/hash_decimal.sql (new) |
| `CometHashExpressionSuite` | 171 | hash - map with decimal (precision >
18) value falls back to Spark | convert | hash/hash_decimal.sql (new) |
| `CometHashExpressionSuite` | 184 | hash - array of integers | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 198 | hash - array of strings | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 213 | hash - array of doubles | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 225 | hash - nested array (array of arrays) |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 237 | hash - struct | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 250 | hash - nested struct | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 262 | hash - null struct with a required
child field | convert | hash/hash_required_child.sql (new) |
| `CometHashExpressionSuite` | 288 | hash - null struct whose child is
itself a struct | convert | hash/hash_required_child.sql (new) |
| `CometHashExpressionSuite` | 313 | hash - list element wrapping a null
struct with a required child field | convert | hash/hash_required_child.sql
(new) |
| `CometHashExpressionSuite` | 356 | hash - struct with array field |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 368 | hash - array of structs | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 380 | hash - map | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 395 | hash - map with complex value type |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 410 | hash - multiple primitive columns |
covered | hash/hash.sql |
| `CometHashExpressionSuite` | 423 | hash - multiple columns with arrays |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 436 | hash - multiple columns with structs |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 449 | hash - empty strings and arrays |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 458 | hash - all nulls | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 467 | hash - with custom seed | covered |
hash/hash.sql |
| `CometHashExpressionSuite` | 477 | hash - large array | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 487 | hash - deeply nested structure |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 504 | hash - with dictionary encoding |
covered | hash/hash.sql |
| `CometHashExpressionSuite` | 517 | hash - array with dictionary encoding |
convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 533 | hash - array of struct | convert |
hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 557 | hash - array of struct not eligible for
batching | convert | hash/hash_nested.sql (new) |
| `CometHashExpressionSuite` | 577 | hash - fuzz test | keep | |
</details>
--
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]
