Copilot commented on code in PR #12965:
URL: https://github.com/apache/gluten/pull/12965#discussion_r3988690281
##########
gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala:
##########
@@ -530,8 +530,6 @@ trait SparkPlanExecApi {
startDate: ExpressionTransformer,
original: DateDiff): ExpressionTransformer
Review Comment:
Removing this method from the shared trait leaves
`BoltSparkPlanExecApi.scala:995` with `override def genCastWithNewChild` but no
method to override, causing the Bolt backend to fail compilation. Retain the
default trait method (or remove/update the Bolt implementation as a coordinated
change).
##########
gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenDataFrameSuite.scala:
##########
@@ -325,54 +325,45 @@ class GlutenDataFrameSuite extends DataFrameSuite with
GlutenSQLTestsTrait {
}
testGluten("Allow leading/trailing whitespace in string before casting") {
- withSQLConf(
- "spark.gluten.velox.castFromVarcharAddTrimNode" -> "true",
- "spark.gluten.bolt.castFromVarcharAddTrimNode" -> "true") {
- def checkResult(df: DataFrame, expectedResult: Seq[Row]): Unit = {
- checkAnswer(df, expectedResult)
- assert(
-
find(df.queryExecution.executedPlan)(_.isInstanceOf[ProjectExecTransformer]).isDefined)
+ 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)
Review Comment:
The Spark 3.5 version of this shared suite is enabled for Bolt, whose cast
path still requires `spark.gluten.bolt.castFromVarcharAddTrimNode` to match
Spark for these inputs. The previous test enabled that setting; removing it
means the Bolt run will compare trimmed Spark results with untrimmed native
results. Keep the Bolt setting enabled around the Gluten query while removing
only the Velox setting.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -484,13 +484,10 @@ object ExpressionConverter extends SQLConfHelper with
Logging {
}
}
}
- // Add trim node, as necessary.
- val newCast =
- BackendsApiManager.getSparkPlanExecApiInstance.genCastWithNewChild(c)
CastTransformer(
substraitExprName,
- replaceWithExpressionTransformer0(newCast.child, attributeSeq,
expressionsMap),
- newCast)
+ replaceWithExpressionTransformer0(c.child, attributeSeq,
expressionsMap),
+ c)
Review Comment:
Calling the cast transformer with `c` directly also bypasses Bolt's
remaining `genCastWithNewChild` implementation. Bolt still exposes
`spark.gluten.bolt.castFromVarcharAddTrimNode` and its override adds the
Spark-compatible trim when enabled, so that setting becomes ineffective; retain
the backend hook here, which is a no-op for Velox after its override is removed.
--
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]
