andygrove opened a new issue, #6632:
URL: https://github.com/apache/datafusion-comet/issues/6632
Part of #6615.
## What / Why
This issue moves window function coverage from `exec/CometWindowExecSuite`
to Comet SQL tests under `sql-tests/windows/` and
`sql-tests/expressions/window/`. It also covers the floating-point sort-key
tests in `CometExpressionSuite`, which target the same order-key fixtures.
Static analysis on `main` at 3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometExpressionSuite` | 0 | 5 | 0 | 0 | 2 | 7 |
| `CometWindowExecSuite` | 25 | 30 | 0 | 2 | 0 | 57 |
| **Total** | **25** | **35** | **0** | **2** | **2** | **64** |
Unlike most areas, 25 of the window tests are **already duplicated** by
`windows/window_functions.sql` and `expressions/window/lag_lead.sql`. They can
be deleted as soon as this issue starts; the table cites the covering fixture
section for each. The rest convert, with two framework facts to know first.
1. **Write `N PRECEDING` as `-N FOLLOWING` in ROWS frames.**
- Spark parses `N PRECEDING` as `UnaryMinus(Literal(N))`, and
`WindowEvaluatorFactoryBase.createBoundOrdering` accepts only an
`IntegerLiteral` as a ROWS bound. The Scala tests get `Literal(-N)` only
because ConstantFolding runs there. In a fixture, the Spark side of the
comparison throws "Unhandled bound in windows expressions".
- `-N FOLLOWING` parses straight to `Literal(-N)`, the same bound the
Scala run ends up with. CometWindowExec reads ROWS bounds with `e.eval()`, so
the native frame is identical.
- RANGE frames are not affected.
- This is a static conclusion. **Confirm it with one fixture first.** If
it fails, L590, L904, L924 and L1497 wait on #6618.
2. **PERCENT_RANK and CUME_DIST are compared only under tolerance today**,
in `window_functions.sql` 1.6, 5.2 and 5.3. Those tolerance queries have no
ORDER BY, so they depend on both engines collecting rows in the same order
(#6616). The Scala tests compare exactly and pass, so use an exact `query`.
## Work
Line numbers refer to `CometWindowExecSuite` unless marked.
**`windows/window_functions.sql` (existing): 18 tests**
- **New small tables:**
- L389: `(value int)` = 1,2,4,3,2,1 with `SUM(value) OVER (ORDER BY value
DESC RANGE BETWEEN UNBOUNDED PRECEDING AND 1 FOLLOWING)`.
- L408: the same table, with `RANGE BETWEEN 1 FOLLOWING AND UNBOUNDED
FOLLOWING` as `expect_fallback(Unsupported RANGE frame lower offset)`.
- L443: `(k int, v boolean)` with `every(v) OVER (PARTITION BY k ORDER BY
v)`.
- L459: the nine testData rows including NULL keys, with a `count(val)`
over a `ROWS CURRENT ROW` frame.
- L482: `count(key)` over `RANGE BETWEEN CURRENT ROW AND 2147483648
FOLLOWING` and over `RANGE BETWEEN 2147483649 PRECEDING AND CURRENT ROW`.
- **New queries on `emp`:**
- L639: `SUM(salary) OVER (PARTITION BY dept)`. Today no native-mode `OVER
(PARTITION BY x)` without ORDER BY is tested; sections 6.3-6.6 all expect
fallback.
- L852, L944, L964: COUNT(*) and MAX over ROWS frames.
- L1208: `FIRST_VALUE` with the default frame. Order by `id`: every
partition has tied `b` values with different `c`, so the Scala query is
nondeterministic.
- L1229: `LAST_VALUE` with RESPECT NULLS.
- L1250: `NTH_VALUE(salary, 2)`.
- L1044, L1270, L1517: one exact query with ROW_NUMBER, RANK, DENSE_RANK,
PERCENT_RANK, CUME_DIST and NTILE(3) in one spec.
- **PRECEDING frames:** L904, L924, L1497, written with `-N FOLLOWING`. Also
correct the section 1.9 comment (L197-205), which says these frames are covered
in Scala "via the DataFrame API".
**New fixtures**
- `windows/window_decimal_aggregates.sql` + `_ansi.sql` (L499, L672, L704,
L742, L778)
- Covers decimal SUM/AVG over windows: OVER () on decimal(2,1), and
running AVG/SUM on decimal(10,2) and (8,2).
- The curated running AVG at (9,1), (9,4), (10,2), (11,3) and (11,6)
includes half-up ties at scale s+4.
- Add decimal(38,38) 0.6, 0.6, -0.6, where SUM and `try_sum` give 0.6,
NULL, 0.6. In the ANSI file, `try_sum` is the native sentinel and SUM gets
`expect_error(NUMERIC_VALUE_OUT_OF_RANGE)`, which matches the error classes on
both sides of 4.0.
- Today these shapes are only in `window_filter.sql`, which is gated to
4.2+.
- `windows/window_basic_frames.sql` (L590)
- `-- ConfigMatrix: spark.sql.adaptive.enabled=true,false`.
- `t1(_1, _2) = (i, 10-i)` written as **one file**, because a ROWS frame
without ORDER BY depends on input order.
- 35 queries: COUNT(_1), COUNT(*), MAX, MIN and SUM, each over 7 frame
shapes.
- `windows/window_aggregate_types.sql` (L528)
- A curated replacement for the random `makeParquetFile` columns: tinyint
through bigint, float, double, decimal(5,2)/(18,10)/(38,37), timestamp and date.
- Include whole-row NULLs, plus low- and high-cardinality variants.
- Add `spark.comet.batchSize` and `outputTimestampType` matrices.
- Use exact `query` except float/double SUM, which needs tolerance with a
total ORDER BY.
- `windows/window_literal_partition_order.sql` (L425)
- `sum(product + 1) OVER (PARTITION BY 1 ORDER BY 2)` over Parquet, with a
shuffle-mode matrix.
- This is stronger than the Scala LocalTableScan source, where
`ConvertToLocalRelation` folds `product + 1`.
- `windows/window_repeated_shuffle.sql` (L508)
- `spark.sql.requireAllClusterKeysForDistribution=true`, native shuffle,
and an AQE matrix.
- `lead(...)` over a `REPARTITION(key1)` subquery.
- `window/lag_lead_offset_beyond_rows.sql` (L368): jvm shuffle and
localTableScan on, with both of the test's queries verbatim.
- **Floating-point keys:**
- `windows/window_group_limit_float_peers.sql` (L126, `MinSparkVersion:
3.5`): signed-zero and sign-bit-NaN peers at the group-limit cutoff, written
with `IF(neg, -x, x)`, with a strictFloatingPoint matrix.
- `windows/nested_float_order_keys.sql` / `_strict.sql` (existing; L213,
and `CometExpressionSuite` L282 and L290)
- RANK/DENSE_RANK over `array(f)`, `array(d)` and `named_struct(...)`
keys, with signed zeros at the `rnk <= 1` cutoff.
- Add array and struct float columns read from Parquet, including
multi-element, empty and NULL arrays.
- `windows/scalar_float_order_keys_strict.sql` (L276, and
`CometExpressionSuite` L96, L158 (16 instances) and L177)
- strictFloatingPoint=true with the SortOrder opt-in off.
- Curated ±0.0, NaN, ±Inf, NULL, Min and Max values with a unique id.
- Cover ORDER BY, TopK, and NULLS FIRST/LAST.
- `windows/window_float_partition_keys.sql` +
`window_group_limit_float_partition_keys.sql` (L315, the latter gated 3.5):
keys 1.0, 2.0, NULL, 0.0, -0.0, NaN and a sign-bit NaN.
## Stays in Scala
- **NaN payload bits.** Parquet writes floats with `floatToIntBits` /
`doubleToLongBits`, dropping the NaN sign and payload, and the SQL comparator
canonicalizes NaN.
- The kept half of L126: the payload peers and `assertRawBits`.
- `CometExpressionSuite` L198 and L301.
- **The ANSI SUM half of L778**, which uses `checkSparkError` (#6617).
## Notes
- **No-op config.** The suite-wide
`spark.comet.operator.WindowExec.allowIncompatible=true` (L51) and the same key
in `window/lag_lead.sql` do nothing. CometWindowExec does not override
`getSupportLevel`, so the operator is always Compatible.
- **Misnamed tests.**
- L443 ("fall back to Spark when ...") actually asserts a fully native
plan. It also creates a view named `test_agg` inside `withTempView("test")`, so
the view is never dropped.
- L499 ("Unsupported window expression should fall back to Spark") never
compares with Spark or checks for a fallback.
- L1208 ("FIRST_VALUE with default ignore nulls") runs with the default,
which is RESPECT NULLS.
- **Redundant loops.** L528 makes twelve runs that cover four distinct cases.
- **Internal duplicates.** L1517 subsumes L984, L1004, L1024, L1044 and
L1270.
- **Stale fixture header.** `window_group_limit_rank_dense_rank.sql` says
its `rk = 0` query uses `spark_answer_only`, but it is a plain `query`.
## 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 |
|---|---|---|---|---|
| `CometExpressionSuite` | 96 | sort floating point with negative zero |
convert | ../windows/scalar_float_order_keys_strict.sql (new) |
| `CometExpressionSuite` | 158 | strict floating point: scalar sort on
$label (loop: col f\|d x ASC\|DESC x NULLS FIRST\|NULLS LAST x compound
false\|true = 16 instances) | convert |
../windows/scalar_float_order_keys_strict.sql (new) |
| `CometExpressionSuite` | 177 | strict floating point: scalar TopK sort key
| convert | ../windows/scalar_float_order_keys_strict.sql (new) |
| `CometExpressionSuite` | 198 | strict floating point: scalar sort keeps
NaN payloads and zero signs unchanged | keep | |
| `CometExpressionSuite` | 282 | sort array of floating point with negative
zero | convert | ../windows/nested_float_order_keys_strict.sql +
../windows/nested_float_order_keys.sql |
| `CometExpressionSuite` | 290 | sort struct containing floating point with
negative zero | convert | ../windows/nested_float_order_keys_strict.sql +
../windows/nested_float_order_keys.sql |
| `CometExpressionSuite` | 301 | strict floating point: nested sort keeps
NaN payloads and zero signs unchanged | keep | |
| `CometWindowExecSuite` | 126 | window group limit: $orderColumn NaN and
signed zero peers at the cutoff (strictFloatingPoint=$strictFloatingPoint)
[loop: orderColumn f\|d x strictFloatingPoint false\|true = 4 instances] |
split (rest: #6618) | ../windows/window_group_limit_float_peers.sql (new) |
| `CometWindowExecSuite` | 213 | window group limit: floating-point values
nested in the order key | convert | ../windows/nested_float_order_keys.sql +
../windows/nested_float_order_keys_strict.sql |
| `CometWindowExecSuite` | 276 | window: scalar floating-point order key
under strict floating point ($orderColumn) [loop: f\|d = 2 instances] | convert
| ../windows/scalar_float_order_keys_strict.sql (new) |
| `CometWindowExecSuite` | 315 | window: floating partition keys
($partitionColumn, $function, group limit=$groupLimit) [loop: f\|d x {RANK()
false, PERCENT_RANK() false, NTILE(2) false, RANK() true} = 8 instances] |
convert | ../windows/window_float_partition_keys.sql (new) +
../windows/window_group_limit_float_partition_keys.sql (new) |
| `CometWindowExecSuite` | 368 | lead/lag should return the default value if
the offset row does not exist | convert |
window/lag_lead_offset_beyond_rows.sql (new) |
| `CometWindowExecSuite` | 389 | window query with rangeBetween | convert |
../windows/window_functions.sql |
| `CometWindowExecSuite` | 408 | window query with rangeBetween FOLLOWING
lower bound falls back to Spark | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 425 | window function: partition and order
expressions | convert | ../windows/window_literal_partition_order.sql (new) |
| `CometWindowExecSuite` | 443 | fall back to Spark when the partition spec
and order spec are not the same for window function | convert |
../windows/window_functions.sql |
| `CometWindowExecSuite` | 459 | Native window operator should be
CometUnaryExec | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 482 | Window range frame with long boundary
should not fail | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 499 | Unsupported window expression should fall
back to Spark | convert | ../windows/window_decimal_aggregates.sql (new) |
| `CometWindowExecSuite` | 508 | Repeated shuffle exchange don't fail |
convert | ../windows/window_repeated_shuffle.sql (new) |
| `CometWindowExecSuite` | 528 | aggregate window function for all types |
convert | ../windows/window_aggregate_types.sql (new) |
| `CometWindowExecSuite` | 590 | Windows support | convert |
../windows/window_basic_frames.sql (new) |
| `CometWindowExecSuite` | 623 | window: simple COUNT(*) without frame |
covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 639 | window: simple SUM with PARTITION BY |
convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 655 | window: AVG with PARTITION BY and ORDER BY
| covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 672 | window: decimal AVG with PARTITION BY and
ORDER BY | convert | ../windows/window_decimal_aggregates.sql (new) |
| `CometWindowExecSuite` | 704 | window: decimal AVG fuzz with PARTITION BY
and ORDER BY [loop over (precision, scale) in (9,1),(9,4),(10,2),(11,3),(11,6)
inside one test] | convert | ../windows/window_decimal_aggregates.sql (new) |
| `CometWindowExecSuite` | 742 | window: decimal SUM with PARTITION BY and
ORDER BY | convert | ../windows/window_decimal_aggregates.sql (new) |
| `CometWindowExecSuite` | 778 | window: decimal SUM recovers from an
intermediate overflow in an expanding frame [loop over ANSI false/true inside
one test] | split (rest: #6617) | ../windows/window_decimal_aggregates.sql
(new) + ../windows/window_decimal_aggregates_ansi.sql (new) |
| `CometWindowExecSuite` | 831 | window: MIN and MAX with ORDER BY | covered
| ../windows/window_functions.sql |
| `CometWindowExecSuite` | 852 | window: COUNT with ROWS BETWEEN UNBOUNDED
PRECEDING AND CURRENT ROW | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 884 | window: SUM with ROWS BETWEEN CURRENT ROW
AND UNBOUNDED FOLLOWING | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 904 | window: AVG with ROWS BETWEEN 1 PRECEDING
AND 1 FOLLOWING | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 924 | window: SUM with ROWS BETWEEN 2 PRECEDING
AND CURRENT ROW | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 944 | window: COUNT with ROWS BETWEEN CURRENT ROW
AND 2 FOLLOWING | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 964 | window: MAX with ROWS BETWEEN UNBOUNDED
PRECEDING AND UNBOUNDED FOLLOWING | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 984 | window: ROW_NUMBER with PARTITION BY and
ORDER BY | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1004 | window: RANK with PARTITION BY and ORDER
BY | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1024 | window: DENSE_RANK with PARTITION BY and
ORDER BY | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1044 | window: PERCENT_RANK with PARTITION BY and
ORDER BY | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1064 | window: NTILE with PARTITION BY and ORDER
BY | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1091 | window: LAG with default offset | covered
| ../windows/window_functions.sql + window/lag_lead.sql |
| `CometWindowExecSuite` | 1111 | window: LAG with offset 2 and default
value | covered | ../windows/window_functions.sql + window/lag_lead.sql |
| `CometWindowExecSuite` | 1131 | window: LAG with IGNORE NULLS | covered |
../windows/window_functions.sql + window/lag_lead.sql |
| `CometWindowExecSuite` | 1149 | window: LEAD with default offset | covered
| ../windows/window_functions.sql + window/lag_lead.sql |
| `CometWindowExecSuite` | 1169 | window: LEAD with offset 2 and default
value | covered | ../windows/window_functions.sql + window/lag_lead.sql |
| `CometWindowExecSuite` | 1189 | window: LEAD with IGNORE NULLS | covered |
../windows/window_functions.sql + window/lag_lead.sql |
| `CometWindowExecSuite` | 1208 | window: FIRST_VALUE with default ignore
nulls | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1229 | window: LAST_VALUE with ROWS frame |
convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1250 | window: NTH_VALUE with position 2 |
convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1270 | window: CUME_DIST with PARTITION BY and
ORDER BY | convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1290 | window: multiple window functions in
single query | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1313 | window: different window specifications in
single query | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1335 | window: ORDER BY DESC with aggregation |
covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1355 | window: multiple PARTITION BY columns |
covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1375 | window: multiple ORDER BY columns |
covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1395 | window: RANGE BETWEEN with numeric ORDER
BY | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1415 | window: RANGE BETWEEN UNBOUNDED PRECEDING
AND CURRENT ROW | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1435 | window: complex expression in window
function | covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1455 | window: window function with WHERE clause
| covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1476 | window: window function with GROUP BY |
covered | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1497 | window: ROWS BETWEEN with negative offset
| convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1517 | window: all ranking functions together |
convert | ../windows/window_functions.sql |
| `CometWindowExecSuite` | 1542 | window: FIRST_VALUE/LAST_VALUE/NTH_VALUE
IGNORE NULLS with leading nulls | covered | ../windows/window_functions.sql |
</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]
