nsivabalan commented on PR #19082:
URL: https://github.com/apache/hudi/pull/19082#issuecomment-4859626131
💬 **Follow-up on the `TestSparkSqlCoreFlow` param trim — clarification and a
small ask.**
Wanted to double-check what actually changes here. On master,
`TestSparkSqlCoreFlow` is class-annotated `@SparkSQLCoreFlow`, which is listed
in `hudi-spark/pom.xml`'s `<tagsToExclude>` — and no CI job sets a matching
`<tagsToInclude>`. So on master the whole suite is **dead-wired**: 48 param
permutations existed in the source (16 core-flow + 32 immutable-user) but ran
on **zero** Spark versions.
This PR revives the suite by re-tagging it with the new `SparkCoreFlow`
scalatest tag and running it in the new `test-spark-core-tests` job on every
Spark version. Great — that's a net gain from 0 → running. But the param matrix
itself is trimmed from 48 → 11 (8 core-flow + 3 immutable-user) in the same PR,
so what we're actually landing is:
| | On master today | With this PR |
| --- | --- | --- |
| Runs on | `spark3.3`, `3.4`, `3.5`, `4.0`, `4.1`, `4.2` | `spark3.3`,
`3.4`, `3.5`, `4.0`, `4.1`, `4.2` |
| Param permutations | 0 (dead-wired) | 12 (subset of the 48 that live in
code) |
So this is unambiguously a net-positive change and **not a coverage
regression** — the 37 permutations that were dropped from
`paramsForImmutable`/`params` weren't running before either. Sorry for calling
this a coverage gap in my original review.
That said, the trim itself is a design choice, and now that the suite is
actually going to run, it's worth asking whether the 11 kept permutations are
the ones most likely to catch a per-Spark-version regression. A few
observations that might help you refine the picks:
1. **`params` (core-flow, 8 kept of 16)**: keeps both table types × both
keygens × both index families (`GLOBAL_*` and non-global) ×
`metadata=true|false`. Looks like a reasonable pairwise spread — one
permutation per (tableType, keygen, indexType) triple with metadata alternating
on/off across the pairs.
2. **`paramsForImmutable` (4 kept of 32)**: this one is much more aggressive.
- `COPY_ON_WRITE|insert|false|SimpleKeyGenerator|GLOBAL_BLOOM`
- `COPY_ON_WRITE|bulk_insert|true|NonpartitionedKeyGenerator|SIMPLE`
- `MERGE_ON_READ|insert|false|SimpleKeyGenerator|GLOBAL_BLOOM`
- `MERGE_ON_READ|bulk_insert|true|NonpartitionedKeyGenerator|SIMPLE`
The coverage is asymmetric: `insert` is only tested on
`SimpleKeyGenerator + GLOBAL_BLOOM`, and `bulk_insert` only on
`NonpartitionedKeyGenerator + SIMPLE`. So we don't have any single row that
tests `insert` with `NonpartitionedKeyGenerator`, or `bulk_insert` with
`SimpleKeyGenerator`, or `insert` with a non-global index — those combinations
run on zero permutations across the whole immutable-user suite.
Suggested tweak: swap or add one row so that `(writeOp × keygen)` is
covered pairwise. Something like:
- Replace one existing row with
`MERGE_ON_READ|insert|true|NonpartitionedKeyGenerator|BLOOM` (adds `insert ×
NonpartitionedKeyGenerator × non-global`).
- Or replace with
`COPY_ON_WRITE|bulk_insert|false|SimpleKeyGenerator|GLOBAL_SIMPLE` (adds
`bulk_insert × SimpleKeyGenerator`).
3. **Comment on the source is slightly misleading now**: the code comment
says the dropped permutations "remain covered by the full suite that runs on
the latest Spark versions" — but the full suite still has `SparkCoreFlow` in
`tagsToExclude` (that's how the `test-spark-scala-*` jobs avoid
double-running). So the dropped 37 permutations don't run in the full-suite job
either. Suggest changing the comment to something like *"trimmed from the
48-permutation matrix in code to a representative spread; the untrimmed
permutations do not run on any version"*, so a future reader doesn't chase a
phantom safety net.
None of this blocks the PR — it's still a strict improvement. Just worth
spending a minute picking better representative permutations while we're
touching this list, since it'll be the only version-cross-cutting SQL-flow
coverage on non-latest versions.
--
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]
