[VL] Remove redundant string trimming before Velox casts - #12965
Conversation
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟡 Changes recommended
The build helper script now defaults to applying a specific upstream Velox PR patch, which can unintentionally affect reproducibility and CI stability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes Gluten-side string trimming logic that was previously injected before CAST-from-varchar when running on the Velox backend, relying instead on Velox’s Spark-compatible cast trimming behavior.
Changes:
- Removes the Velox-specific CAST trim-node injection path (and its plumbing) from Gluten’s cast conversion/execution APIs.
- Deletes the associated Velox config (
spark.gluten.velox.castFromVarcharAddTrimNode) and removes it from documentation. - Updates Spark 3.5 / 4.0 / 4.1 Gluten UTs to validate whitespace-tolerant casts without enabling the removed config.
File summaries
| File | Description |
|---|---|
| gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenDataFrameSuite.scala | Drops use of the removed Velox trim config while keeping cast-with-whitespace assertions. |
| gluten-ut/spark40/src/test/scala/org/apache/spark/sql/GlutenDataFrameSuite.scala | Same test update for Spark 4.0 profile. |
| gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenDataFrameSuite.scala | Same test update for Spark 3.5 profile. |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala | Stops generating a backend-modified Cast for trimming; uses the original Cast child directly. |
| gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala | Removes the now-unused genCastWithNewChild hook from the backend API. |
| ep/build-velox/src/get-velox.sh | Sets a non-empty default UPSTREAM_VELOX_PR_ID, which forces patch application in builds. |
| docs/velox-configuration.md | Removes documentation for the deleted Velox trimming config. |
| backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala | Removes the config accessor and registry entry for CAST-from-varchar trim-node injection. |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala | Deletes Velox’s override that injected StringTrim nodes ahead of casts. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🔵 Needs a closer look
The build script change pins UPSTREAM_VELOX_PR_ID to a specific external PR by default, making builds non-reproducible and CI fragile.
Review details
Suppressed comments (1)
ep/build-velox/src/get-velox.sh:28
- UPSTREAM_VELOX_PR_ID is meant for developer-only testing, but setting a non-empty default means every build will silently download and apply an external GitHub PR patch, making builds non-reproducible and potentially breaking CI when that PR changes/disappears. This should default to empty and only be set explicitly by developers when needed.
# Developer use only for testing Velox PR.
UPSTREAM_VELOX_PR_ID="18821"
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
| def checkResult(sql: String): Unit = { | ||
| var expected: Seq[Row] = null | ||
| withSQLConf(GlutenConfig.GLUTEN_ENABLED.key -> "false") { | ||
| expected = spark.sql(sql).collect() | ||
| } | ||
|
|
||
| // scalastyle:off nonascii | ||
| Seq( | ||
| " 123", | ||
| "123 ", | ||
| " 123 ", | ||
| "\u2000123\n\n\n", | ||
| "123\r\r\r", | ||
| "123\f\f\f", | ||
| "123\u000C", | ||
| "123\u0000") | ||
| .toDF("col1") | ||
| .createOrReplaceTempView("t1") | ||
| // scalastyle:on nonascii | ||
| val expectedIntResult = Row(123) :: Row(123) :: | ||
| Row(123) :: Row(123) :: Row(123) :: Row(123) :: Row(123) :: Row(123) :: Nil | ||
| var df = spark.sql("select cast(col1 as int) from t1") | ||
| checkResult(df, expectedIntResult) | ||
| df = spark.sql("select cast(col1 as long) from t1") | ||
| checkResult(df, expectedIntResult) | ||
|
|
||
| Seq(" 123.5", "123.5 ", " 123.5 ", "123.5\n\n\n", "123.5\r\r\r", "123.5\f\f\f", "123.5\u000C") | ||
| .toDF("col1") | ||
| .createOrReplaceTempView("t1") | ||
| val expectedFloatResult = Row(123.5) :: Row(123.5) :: | ||
| Row(123.5) :: Row(123.5) :: Row(123.5) :: Row(123.5) :: Row(123.5) :: Nil | ||
| df = spark.sql("select cast(col1 as float) from t1") | ||
| checkResult(df, expectedFloatResult) | ||
| df = spark.sql("select cast(col1 as double) from t1") | ||
| checkResult(df, expectedFloatResult) | ||
|
|
||
| // scalastyle:off nonascii | ||
| val rawData = | ||
| Seq(" abc", "abc ", " abc ", "\u2000abc\n\n\n", "abc\r\r\r", "abc\f\f\f", "abc\u000C") | ||
| // scalastyle:on nonascii | ||
| rawData.toDF("col1").createOrReplaceTempView("t1") | ||
| val expectedBinaryResult = rawData.map(d => Row(d.getBytes(StandardCharsets.UTF_8))).seq | ||
| df = spark.sql("select cast(col1 as binary) from t1") | ||
| checkResult(df, expectedBinaryResult) | ||
| val df = spark.sql(sql) | ||
| checkAnswer(df, expected) | ||
| assert(find(df.queryExecution.executedPlan)(_.isInstanceOf[ProjectExecTransformer]).isDefined) |
|
Run Gluten Clickhouse CI on x86 |
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
Looks good. Please rebase the code and resolve the code conflicts.
|
Run Gluten Clickhouse CI on x86 |
|
@philo-he Thanks for the review. This PR is currently based on facebookincubator/velox#18821 and will need to drop the PR number change after it gets merged. |
There was a problem hiding this comment.
🟡 Changes recommended
The review identifies a critical Bolt compilation issue and additional backend, test, and build issues that must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
ep/build-velox/src/get-velox.sh:28
- This variable is explicitly marked for developer-only Velox PR testing, but setting it here makes every Velox source setup download and apply PR #18821 through
apply_provided_velox_patch. That makes normal builds depend on an external patch and can fail when the patch no longer applies; please leave the default empty.
UPSTREAM_VELOX_PR_ID="18821"
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
| @@ -530,8 +530,6 @@ trait SparkPlanExecApi { | |||
| startDate: ExpressionTransformer, | |||
| original: DateDiff): ExpressionTransformer | |||
|
|
|||
| CastTransformer( | ||
| substraitExprName, | ||
| replaceWithExpressionTransformer0(newCast.child, attributeSeq, expressionsMap), | ||
| newCast) | ||
| replaceWithExpressionTransformer0(c.child, attributeSeq, expressionsMap), | ||
| c) |
| val df = spark.sql(sql) | ||
| checkAnswer(df, expected) | ||
| assert( | ||
| find(df.queryExecution.executedPlan)(_.isInstanceOf[ProjectExecTransformer]).isDefined) |
What changes are proposed in this pull request?
This patch removes the extra string trimming in Gluten before delegating casts to Velox.
Velox already applies Spark-compatible string cast trimming in its Spark cast hooks.
The corresponding Velox behavior is tracked with SPARK-59182, which discusses whether Spark should align the behavior of the two string cast trimming paths.
Depends on facebookincubator/velox#18821.
How was this patch tested?
Unit tests