Copilot commented on code in PR #12965:
URL: https://github.com/apache/gluten/pull/12965#discussion_r3969922762
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenDataFrameSuite.scala:
##########
@@ -324,52 +325,44 @@ class GlutenDataFrameSuite extends DataFrameSuite with
GlutenSQLTestsTrait {
}
testGluten("Allow leading/trailing whitespace in string before casting") {
- withSQLConf("spark.gluten.velox.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 helper uses a mutable `var expected` initialized to `null`, which is
brittle and makes future edits more error-prone. Prefer a single-expression
`val expected = withSQLConf(...) { spark.sql(sql).collect().toSeq }` (and drop
the `null`), which is clearer and avoids accidental NPEs. The same pattern
appears in the Spark40/35/34 variants of this suite.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -25,7 +25,7 @@ RUN_SETUP_SCRIPT=ON
ENABLE_ENHANCED_FEATURES=OFF
# Developer use only for testing Velox PR.
-UPSTREAM_VELOX_PR_ID=""
+UPSTREAM_VELOX_PR_ID="18821"
Review Comment:
This script comment says “Developer use only for testing Velox PR.” Setting
a non-empty default PR ID in-repo will make default builds unexpectedly pull a
specific upstream PR, which can break reproducibility and CI stability. Please
revert to an empty default (or gate it behind an explicit environment
variable/flag) so normal builds track the intended baseline.
--
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]
