LuciferYang commented on issue #12863: URL: https://github.com/apache/gluten/issues/12863#issuecomment-5407196332
Answered. Removing `gluten-ut/spark33` does not drop coverage that Gluten actually validated, and the four cases where something was really lost now have PRs. Details below so nobody has to redo the diff. ### Method, and one correction worth knowing about Computed the enabled-case set per version module from `VeloxTestSettings` / `ClickHouseTestSettings` the way this issue describes, then diffed spark33's set against the union of 3.4, 3.5, 4.0 and 4.1. That produced 39 candidates: 33 on Velox across 17 suites, 6 on ClickHouse across 4 suites. The correction: a settings diff only tells you a name is excluded everywhere, not that the case ever existed on 3.3. Deciding that requires resolving the Gluten wrapper's parent Spark class and then checking that class in **Spark 3.3.1**, which is what `<spark.version>` is under the `spark-3.3` profile. Checking against a later 3.3.x gives wrong answers, because Spark backports test cases into maintenance releases. Two candidates flipped on exactly this: `SPARK-42635: timestampadd near daylight saving transition` and `row group skipping doesn't overflow when reading into larger type` both exist in 3.3.4 and neither exists in 3.3.1, so in this repository they never ran on 3.3. 3.3.1 has no `-test-sources.jar`, but case names are compiled into the class files as string constants, so `unzip -p <tests.jar> '*/<ParentClass>.class' | strings -a` settles it per class. ### Suite granularity: no gap Four suites are enabled on 3.3 and on none of the surviving versions: - `GlutenCastSuiteWithAnsiModeOn`, `GlutenAnsiCastSuiteWithAnsiModeOn`, `GlutenAnsiCastSuiteWithAnsiModeOff`. None of the three reaches Velox on CI as configured, so deleting them loses no backend coverage, but the reason differs per suite and is worth writing down. All three live in `gluten-ut/spark33/.../GlutenAnsiCastSuite.scala` and set session ANSI in their own `beforeAll`. `GlutenCastSuiteWithAnsiModeOn` sets it true and builds plain `Cast`, so it depends entirely on `FallbackOnANSIMode` (`FallbackRules.scala:30`, `enableAnsiMode && enableAnsiFallback`) tagging the whole plan; `velox_backend_x86.yml` contains no `ansiFallback` anywhere and the spark33 jobs pass only `-Dspark.test.home=...`, so `enableAnsiFallback` stays at its default of true. `GlutenAnsiCastSuiteWithAnsiModeOn` is covered by the same path and additionally builds `AnsiCast`. `GlutenAnsiCastSuiteWithAnsiModeOff` sets ANSI false, so that path does not apply to it; it never reaches Velox because `AnsiCast` is a 3.3-only class with no `Sig` entry (`ExpressionMappings.scala:45` has only `Sig[Cast](CAST)`, and the main code has zero references to `AnsiCast`), and the mapping is keyed by class. On top of all three, `GlutenTestsTrait.scala:50` defines `ansiTest = !enableAnsiFallback`, which is false here, so `checkExceptionInExpression` delegates to Spark's `ExpressionEvalHelper` without building a DataFrame at all. - `GlutenDataSourceV2SQLSuite`: upstream split it into `GlutenDataSourceV2SQLSuiteV1Filter` and `...V2Filter` in 3.4, and both are enabled on all four versions. Same coverage, different name. ### Case granularity: 39 candidates, all resolved 25 of the 33 Velox candidates and 2 of the 6 ClickHouse candidates are cases that do not exist in Spark 3.3.1, so they never ran on 3.3 and cannot be a gap. The ticket numbers make most of them obvious: SPARK-39557, 39731, 39749, 41017, 41048, 42635, 44641, 45604, 45786, plus the DEFAULT-value INSERT cases that arrived with 3.4, `filter pushdown - StringPredicate`, `row group skipping doesn't overflow when reading into larger type`, and `CONVERSION_INVALID_INPUT: to_binary conversion function hex`. Two of those deserve a note because a name diff makes them look like losses when they are renames. `UNSUPPORTED_FEATURE - SPARK-36346: can't read Timestamp as TimestampNTZ` exists from 3.4 on; 3.3 has the same scenario as `UNSUPPORTED_OPERATION - SPARK-36346: ...`, which `gluten-ut/spark33` also excludes, with the same reason. `ordering and partitioning reporting` is likewise 3.4+; 3.3 has `partitioning reporting`, which spark33 excludes and rewrites in its own `GlutenDataSourceV2Suite`, exactly as the surviving modules do. That leaves 12 candidates that really did run on 3.3: | case | suite | outcome | |-|-|-| | ANSI out-of-range byte / short / int / long | VL `GlutenTryCastSuite` | pass once enabled, removed in #12886 | | cast from invalid string to numeric should throw NumberFormatException | VL `GlutenTryCastSuite` | same | | SPARK-26218: Fix the corner case of codegen when casting float to Integer | VL `GlutenTryCastSuite` | same | | Process Infinity, -Infinity, NaN in case insensitive manner | VL `GlutenTryCastSuite` | `+inf not supported in folly`, backend limit, applies on 3.3 too | | cast from timestamp II | VL `GlutenTryCastSuite` | rewritten for ANSI mode, comment already there | | data type casting | VL `GlutenTryCastSuite` | timezone set through config, `testGluten` rewrite already there | | SPARK-35106: Throw exception when rename custom partition paths returns false | VL `GlutenInsertSuite` | native write staging dir differs from vanilla, not version-specific | | Write Spark version into Parquet metadata | VL `GlutenParquetIOSuite` | Velox does not write file metadata into Parquet, backend limit | | Stop task set if FileAlreadyExistsException was thrown | VL `GlutenInsertSuite` | fails; reason now written down in #12888 | | cast from timestamp II | VL `GlutenCastSuite` | 4.0/4.1 had a rewrite, 3.4/3.5 did not; ported in #12888 | | cast from array II / III, cast from struct III | CH `GlutenTryCastSuite` | only ran on CH 3.3; #12889 | | tanh | CH `GlutenMathExpressionsSuite` | only ran on CH 3.3; #12889 | For `Stop task set if FileAlreadyExistsException was thrown` I removed the exclusion and ran `GlutenInsertSuite` against 3.4.4 with Velox: 70 of 71 pass and this one fails with `Expected exception org.apache.spark.SparkException to be thrown, but org.apache.hadoop.fs.FileAlreadyExistsException was thrown`. The case installs a `FileSystem` that throws on create and expects Spark's write path to wrap the failure; Gluten's native write surfaces the raw exception. So the exclusion was right and only its reason was missing. ### One more axis worth recording here While inventorying this I also settled the `.sql` file list, which is a different granularity but the same question. Ten files appear only in `gluten-ut/spark33`'s `SUPPORTED_SQL_QUERY_LIST`; one of them is in that module's own `ignoreList`, so nine would stop running. All nine pass on 3.4.4 locally, and #12890 adds them to the four surviving modules. And `SPARK-33338: GROUP BY using literal map should not fail`, a Gluten-authored case that only really runs on spark33, turns out to be a deliberate disable rather than a missed migration: all four surviving modules carry it as `ignore(...)` with the reason above it, `// Velox throw exception : An unsupported nested encoding was found.`, added in `fd33a939a` when the native Parquet writer landed for Spark 3.4. ### Two limitations of the method A name diff reports a false gap when upstream renames a case or a suite and Gluten follows the rename. That happened twice here, with the `GlutenDataSourceV2SQLSuite` split and with `CastSuite` becoming `CastWithAnsiOffSuite` upstream while Gluten kept `GlutenCastSuite` on 3.4/3.5 and switched to `GlutenCastWithAnsiOffSuite` on 4.0/4.1. And matching a case name against a version's tests jar as a whole is not the same as finding it in that suite's parent class; the check has to land on the class, and on the right 3.3.x. Closing: this is not a blocker for removing the module. The four real losses are covered by #12888, #12889 and #12890. -- 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]
