[GLUTEN-12863][TEST] Record six reason-less test exclusions, and port the cast from timestamp II rewrite to Spark 3.4/3.5 - #12898
Open
LuciferYang wants to merge 1 commit into
Conversation
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves test maintainability and coverage tracking in gluten-ut by documenting previously reason-less test exclusions and porting an existing Spark 4.x Gluten rewrite for “cast from timestamp II” down to Spark 3.4 and 3.5 suites. It keeps production code unchanged while making exclusion intent explicit and ensuring the non-overflow assertions in that test case are exercised on Spark 3.4/3.5 under Velox.
Changes:
- Add a
testGluten("cast from timestamp II")rewrite to Spark 3.4 and 3.5GlutenCastSuite, aligning behavior with the existing Spark 4.0/4.1 rewrite (dropping only theLong.MinValueassertion due tocollect()timestamp conversion overflow). - Add explicit comments documenting why
cast from timestamp IIand one insert-path exception-wrapping test remain excluded in Velox settings across Spark 3.4/3.5/4.0/4.1. - Add ClickHouse-side exclusions/comments to avoid running the new Gluten rewrite on ClickHouse (and document four existing Spark 3.5 ClickHouse exclusions with concrete failure reasons).
Reviewed changes
Copilot reviewed 10 out of 10 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/spark/sql/catalyst/expressions/GlutenCastWithAnsiOffSuite.scala | Refines the rationale/comments for dropping the Long.MinValue assertion in the existing Spark 4.1 rewrite. |
| gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Adds reasons for two Velox exclusions: “cast from timestamp II” and the FileAlreadyExistsException wrapping mismatch. |
| gluten-ut/spark40/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastWithAnsiOffSuite.scala | Same comment refinement as Spark 4.1 for the existing rewrite. |
| gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Adds reasons for the same two Velox exclusions as Spark 4.1. |
| gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastSuite.scala | Ports the Spark 4.x-style testGluten("cast from timestamp II") rewrite into Spark 3.5. |
| gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Documents why “cast from timestamp II” is excluded (rewrite exists) and why the FileAlreadyExistsException case is excluded. |
| gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala | Excludes the new Gluten rewrite on ClickHouse and documents four ClickHouse Spark 3.5 exclusions with concrete failure behavior. |
| gluten-ut/spark34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastSuite.scala | Ports the Spark 4.x-style testGluten("cast from timestamp II") rewrite into Spark 3.4. |
| gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Adds the same two Velox exclusion reasons as Spark 3.5/4.0/4.1. |
| gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala | Excludes the new Gluten rewrite on ClickHouse (keeps file shape aligned with Spark 3.5). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jackylee-ch
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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