[GLUTEN-12863][TEST] Record six reason-less test exclusions, and port the cast from timestamp II rewrite to Spark 3.4/3.5 - #12888
Conversation
…amp II rewrite to 3.4/3.5
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
This PR documents two previously unexplained VeloxTestSettings.exclude(...) entries and restores partial coverage for "cast from timestamp II" on Spark 3.4/3.5 by porting the existing Spark 4.0/4.1 Gluten-specific rewrite (skipping the known Long.MinValue overflow on the collect()/timestamp conversion path). No production code is changed.
Changes:
- Add explicit rationale comments for excluding
"cast from timestamp II"and"Stop task set if FileAlreadyExistsException was thrown"across Spark 3.4–4.1 Velox settings. - Add
testGluten("cast from timestamp II")in Spark 3.4 and 3.5GlutenCastSuite, mirroring the Spark 4.x rewrite and intentionally skippingLong.MinValue.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Adds missing explanations for two excludes in Spark 4.1 Velox test settings. |
| gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Adds missing explanations for two excludes in Spark 4.0 Velox test settings. |
| gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastSuite.scala | Re-enables most assertions for "cast from timestamp II" via a Gluten rewrite that skips Long.MinValue. |
| gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Documents why "cast from timestamp II" and the FileAlreadyExistsException case remain excluded in the vanilla suite. |
| gluten-ut/spark34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastSuite.scala | Re-enables most assertions for "cast from timestamp II" via a Gluten rewrite that skips Long.MinValue. |
| gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Documents why "cast from timestamp II" and the FileAlreadyExistsException case remain excluded in the vanilla suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LuciferYang
changed the title
…op an unsupported claim from the exclude comment
| // the native write staing dir is differnt with vanilla Spark for coustom partition paths | ||
| .exclude("SPARK-35106: Throw exception when rename custom partition paths returns false") | ||
| // The case expects a SparkException; Gluten surfaces the raw | ||
| // FileAlreadyExistsException instead. Reproduced on Spark 3.4.4. |
| // the native write staing dir is differnt with vanilla Spark for coustom partition paths | ||
| .exclude("SPARK-35106: Throw exception when rename custom partition paths returns false") | ||
| // The case expects a SparkException; Gluten surfaces the raw | ||
| // FileAlreadyExistsException instead. Reproduced on Spark 3.4.4. |
| // during rebase. Velox computes correctly; only the collect path fails. Skip Long.MinValue. | ||
| testGluten("cast from timestamp II") { | ||
| checkEvaluation(cast(Double.NaN, TimestampType), null) | ||
| checkEvaluation(cast(1.0 / 0.0, TimestampType), null) | ||
| checkEvaluation(cast(Float.NaN, TimestampType), null) | ||
| checkEvaluation(cast(1.0f / 0.0f, TimestampType), null) | ||
| checkEvaluation(cast(Literal(Long.MaxValue), TimestampType), Long.MaxValue) | ||
| // Skip Long.MinValue: Velox result is correct but collect() path overflows in | ||
| // rebaseGregorianToJulianMicros when converting extreme timestamp to java.sql.Timestamp. |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
| // the native write staing dir is differnt with vanilla Spark for coustom partition paths | ||
| .exclude("SPARK-35106: Throw exception when rename custom partition paths returns false") | ||
| // The case expects a SparkException; Gluten surfaces the raw | ||
| // FileAlreadyExistsException instead. Reproduced on Spark 3.4.4. |
| .excludeCH("SPARK-33291: Cast struct with null elements to string") | ||
| .excludeCH("SPARK-35111: Cast string to year-month interval") | ||
| .excludeCH("Gluten - data type casting") | ||
| // Gluten rewrite of the vanilla case excluded below; not vetted on ClickHouse. |
| .excludeCH("SPARK-35111: Cast string to year-month interval") | ||
| .excludeCH("Gluten - data type casting") | ||
| // Gluten rewrite of the vanilla case excluded below; not vetted on ClickHouse. | ||
| .excludeCH("Gluten - cast from timestamp II") |
| // Gluten's glutenCheckExpression uses collect(), which triggers | ||
| // toJavaTimestamp -> rebaseGregorianToJulianMicros. Long.MinValue micros (~292000 BC) overflows | ||
| // during rebase, so the vanilla case's Long.MinValue assertion is dropped here. |
| .exclude("data type casting") | ||
| // Revised by setting timezone through config and commented unsupported cases. | ||
| .exclude("cast string to timestamp") | ||
| // Rewritten in GlutenCastWithAnsiOffSuite without Long.MinValue, which overflows on the collect path. |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
|
Thanks, five rounds of this got the comments into decent shape. What I took and what I did not: Taken. The version stamp is gone from the Not taken, with reasons. Re-adding the nuance that the failure is in the harness rather than in the Velox cast: that is the claim an earlier comment in this same review series asked me to remove, and rightly. The comment already names Extracting the rewrite body into a shared helper: per-module duplication is what this repo does for rewrites. One correction on a stale comment: |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
LuciferYang
changed the title
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:852
- Fix spelling/grammar in this nearby comment (staging/different/custom; "differs from"), since it sits in the same updated hunk and is easy to correct while touching this block.
// the native write staing dir is differnt with vanilla Spark for coustom partition paths
.exclude("SPARK-35106: Throw exception when rename custom partition paths returns false")
// The case expects a SparkException; Gluten surfaces the raw
// FileAlreadyExistsException instead.
gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:871
- Fix spelling/grammar in this nearby comment (staging/different/custom; "differs from"), since it sits in the same updated hunk and is easy to correct while touching this block.
// the native write staing dir is differnt with vanilla Spark for coustom partition paths
.exclude("SPARK-35106: Throw exception when rename custom partition paths returns false")
// The case expects a SparkException; Gluten surfaces the raw
// FileAlreadyExistsException instead.
gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:601
- Fix spelling/grammar in this nearby comment (staging/different/custom; "differs from"), since it sits in the same updated hunk and is easy to correct while touching this block.
// the native write staing dir is differnt with vanilla Spark for coustom partition paths
.exclude("SPARK-35106: Throw exception when rename custom partition paths returns false")
// The case expects a SparkException; Gluten surfaces the raw
// FileAlreadyExistsException instead.
gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:641
- Fix spelling/grammar in this nearby comment (staging/different/custom; "differs from"), since it sits in the same updated hunk and is easy to correct while touching this block.
// the native write staing dir is differnt with vanilla Spark for coustom partition paths
.exclude("SPARK-35106: Throw exception when rename custom partition paths returns false")
// The case expects a SparkException; Gluten surfaces the raw
// FileAlreadyExistsException instead.
|
Run Gluten Clickhouse CI on x86 |
| .excludeCH("Gluten - data type casting") | ||
| // The Gluten rewrite of "cast from timestamp II" is not vetted on ClickHouse; | ||
| // the vanilla case is excluded separately in this block. | ||
| .excludeCH("Gluten - cast from timestamp II") |
|
Run Gluten Clickhouse CI on x86 |
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.testGluten("cast from timestamp II")rewrite; comments in the Velox settings; a paired exclusion in the ClickHouse settingsSpark 3.4.4 and 3.5.5 both assert six values in
cast from timestamp II, the four NaN/Infinity ones plusLong.MaxValueandLong.MinValue. Spark 3.4.0 only had the four, which is why the exclusion predates the extra assertions. 4.0 and 4.1 carry atestGluten("cast from timestamp II")rewrite inGlutenCastWithAnsiOffSuitethat keeps five of the six and says why the sixth is dropped:glutenCheckExpressionusescollect(), which goes throughtoJavaTimestampandrebaseGregorianToJulianMicros, andLong.MinValuemicroseconds 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
testGlutenregistration 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 neighbouringdata type castingentry 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, sincebackends-clickhouse/pom.xmlhas 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 theSPARK-35106entry between them. I removed the exclusion locally and ranGlutenInsertSuiteagainst Spark 3.4.4 with Velox. 70 of 71 cases pass; this one fails:The case installs a
FileSystemthat throwsFileAlreadyExistsExceptionon 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.scalamirrors both branches ofFileFormatWriter, 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 IIIandcast from struct IIIare 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 whengluten-ut/spark33goes 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: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;
tanhdiffers at the sixth significant digit;cast from array IIthrows 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 failsis 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
GlutenTryCastSuiteblock in all four modules excludescast from timestamp IIwith// Rewrite test for Gluten not supported with ANSI mode, but notestGluten("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-compileandspotless:checkpass on-Pspark-3.4,-Pspark-3.5 -Pscala-2.13,-Pspark-4.0 -Pscala-2.13and-Pspark-4.1 -Pscala-2.13. The five re-enabled assertions on 3.4/3.5 are for CI to confirm; theGlutenInsertSuiterun 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/spark33under #12807. Related: #12863, #12886, #12889.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5