LuciferYang opened a new pull request, #12898:
URL: https://github.com/apache/gluten/pull/12898
Six `.exclude(...)` entries across the Velox and ClickHouse test settings
carried no comment saying what fails. This writes down all six reasons, and one
of them turned out to be a real coverage gap on Spark 3.4 and 3.5. No
production code.
| module | change |
|-|-|
| spark34, spark35 | a `testGluten("cast from timestamp II")` rewrite;
comments in the Velox settings; a paired exclusion in the ClickHouse settings |
| spark35 | reasons for four ClickHouse exclusions, from a CI run |
| spark40, spark41 | comments in the Velox settings; two claims dropped from
the rewrite they already had |
Spark 3.4.4 and 3.5.5 both assert six values in `cast from timestamp II`,
the four NaN/Infinity ones plus `Long.MaxValue` and `Long.MinValue`. Spark
3.4.0 only had the four, which is why the exclusion predates the extra
assertions. 4.0 and 4.1 carry a `testGluten("cast from timestamp II")` rewrite
in `GlutenCastWithAnsiOffSuite` that keeps five of the six and says why the
sixth is dropped: `glutenCheckExpression` uses `collect()`, which goes through
`toJavaTimestamp` and `rebaseGregorianToJulianMicros`, and `Long.MinValue`
microseconds overflows there.
3.4 and 3.5 have the bare exclusion and no rewrite, so on those two versions
the other five assertions run nowhere. This ports the 4.0 rewrite to their
`GlutenCastSuite`, which is safe because both wrappers extend the same parent,
`CastWithAnsiOffSuite`. All four modules also get a comment above the vanilla
exclusion so the pair is visible from the settings file.
Because the wrapper file is shared by both backends and `testGluten`
registration is not backend-conditional, adding the rewrite would also have
started running it under ClickHouse, which has never validated this case. The
ClickHouse settings get the matching exclusion that the neighbouring `data type
casting` entry already uses: `.excludeCH("Gluten - cast from timestamp II")` on
spark35 and `.excludeGlutenTest("cast from timestamp II")` on spark34. Only the
spark35 one has any effect, since `backends-clickhouse/pom.xml` has profiles
for 3.3 and 3.5 only; the spark34 line is there to keep the two files the same
shape.
The other Velox entry, `Stop task set if FileAlreadyExistsException was
thrown`, is excluded in all four modules with nothing said about why; the
comment on the line above it, `the native write staing dir is differnt with
vanilla Spark for coustom partition paths`, belongs to the `SPARK-35106` entry
between them. I removed the exclusion locally and ran `GlutenInsertSuite`
against Spark 3.4.4 with Velox. 70 of 71 cases pass; this one fails:
```
- Stop task set if FileAlreadyExistsException was thrown *** FAILED ***
Expected exception org.apache.spark.SparkException to be thrown,
but org.apache.hadoop.fs.FileAlreadyExistsException was thrown
(InsertSuite.scala:2035)
```
The case installs a `FileSystem` that throws `FileAlreadyExistsException` on
create and expects Spark's write path to wrap it. Gluten surfaces the raw
Hadoop exception instead, and the new comment records only that. Gluten's
native write does have its own task-failure handling:
`VeloxColumnarWriteFilesExec.scala` mirrors both branches of
`FileFormatWriter`, so where the raw exception escapes is not something I
established, and the comment does not guess at it.
The four ClickHouse entries came from #12889, now folded in here since it
was the same activity on a file this PR already touches. `tanh`, `cast from
array II`, `cast from array III` and `cast from struct III` are excluded on
ClickHouse 3.5 with no reason recorded, and all four run on ClickHouse 3.3, so
they would have lost their only run site when `gluten-ut/spark33` goes away
under #12807. That made it worth removing all four and letting the ClickHouse
CI answer whether the exclusions were stale. It failed all four:
```
- tanh *** FAILED ***
Incorrect evaluation: TANH(-0.1), actual: -0.0996695958408681, expected:
-0.09966799462495582
- cast from array II *** FAILED ***
Exception evaluating try_cast([123,true,f] as array<boolean>)
- cast from array III *** FAILED ***
Incorrect evaluation: try_cast([2.147483648E9] as array<int>),
actual: ArraySeq(-2147483648), expected: [Lscala.runtime.Null$;@73684390
- cast from struct III *** FAILED ***
Incorrect evaluation: try_cast([2.147483648E9] as struct<a:int>),
actual: [-2147483648], expected: [null]
```
So those exclusions stay and only their reasons were missing. The two
array/struct cases are one behaviour, TRY-mode overflow inside a complex type
wrapping instead of yielding null; `tanh` differs at the sixth significant
digit; `cast from array II` throws rather than returning null. Whether
ClickHouse should be changed to match is out of scope here.
Two claims came out of the rewrite's own comments while addressing review,
in all four modules rather than only the two this patch adds. `Velox computes
correctly; only the collect path fails` is not something this repository can
show, so the comment now stops at the mechanism it can. The same explanation
also appeared twice, once above the test and once inside it; the inner one is
now a pointer. And the exclude comment no longer names the version it was
reproduced on, since the identical text sits in the 4.0 and 4.1 settings where
no such run was done.
One adjacent thing left alone: the `GlutenTryCastSuite` block in all four
modules excludes `cast from timestamp II` with `// Rewrite test for Gluten not
supported with ANSI mode`, but no `testGluten("cast from timestamp II")` exists
in that suite's wrapper, so that comment points at a rewrite that is not there.
It predates this change, and its real reason is unverified, so replacing one
unsupported sentence with another would not help. It needs the exclusion
removed and the suite run on 3.4 to settle, which is a separate change.
`test-compile` and `spotless:check` pass on `-Pspark-3.4`, `-Pspark-3.5
-Pscala-2.13`, `-Pspark-4.0 -Pscala-2.13` and `-Pspark-4.1 -Pscala-2.13`. The
five re-enabled assertions on 3.4/3.5 are for CI to confirm; the
`GlutenInsertSuite` run above is on this machine, which is not authoritative
for a pass but is enough to read a deterministic exception-type mismatch.
Found while auditing the excludes that have no stated reason, in preparation
for removing `gluten-ut/spark33` under #12807. Related: #12863, #12886, #12889.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5
--
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]
