Skip to content

[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
apache:mainfrom
LuciferYang:spark33-drop-p06-settings-hygiene-bak
Open

[GLUTEN-12863][TEST] Record six reason-less test exclusions, and port the cast from timestamp II rewrite to Spark 3.4/3.5#12898
LuciferYang wants to merge 1 commit into
apache:mainfrom
LuciferYang:spark33-drop-p06-settings-hygiene-bak

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

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

Copilot AI lite review requested due to automatic review settings August 25, 2026 13:13
@github-actions github-actions Bot added the CORE works for Gluten Core label Aug 25, 2026
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.5 GlutenCastSuite, aligning behavior with the existing Spark 4.0/4.1 rewrite (dropping only the Long.MinValue assertion due to collect() timestamp conversion overflow).
  • Add explicit comments documenting why cast from timestamp II and 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CORE works for Gluten Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants