You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "Henry2SS (via GitHub)" <gi...@apache.org> on 2024/10/11 06:57:25 UTC
[PR] [VL] remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Henry2SS opened a new pull request, #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476
## What changes were proposed in this pull request?
remove unnecessary trim node in CAST, when input type is VARCHAR.
(Fixes: \#7475)
## How was this patch tested?
Integration tests passed locally. The performance is 1.x times faster.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "Yohahaha (via GitHub)" <gi...@apache.org>.
Yohahaha commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1810380580
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
```
val expect = Seq(Row(123))
checkAnswer(sql("SELECT cast('\\t\\n\\u001F 123\\u000B' as int);"), expect)
```
this UT works.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406682725
https://github.com/apache/incubator-gluten/issues/7475
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2431104593
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1811988477
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
> @Henry2SS, Spark UT doesn't cover many corner cases used in production workload. I'm afraid users would complain result mismatching issues due to this removement. Agree with @jackylee-ch, it's good to use a config to give the control to users. And maybe at some future time point, we can remove it.
Done.
And thanks for your advice. @PHILO-HE @jackylee-ch @Yohahaha
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1814150195
##########
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala:
##########
@@ -2125,4 +2127,12 @@ object GlutenConfig {
"Otherwise, throw an exception.")
.booleanConf
.createWithDefault(true)
+
+ val CAST_FROM_VARCHAR_ADD_TRIM_NODE =
+ buildConf("spark.gluten.velox.castFromVarcharAddTrimNode")
+ .internal()
+ .doc("If true, will add a trim node which has the same sementic as vanilla Spark." +
+ "Otherwise, do nothing.")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
> need add this config to
I'll do this.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] Add a config to control whether add trim node when CAST from varchar [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE merged PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2431038567
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406744808
> @Henry2SS, thanks for your pr! This piece of scala code is for making Gluten/Velox consistent with Spark. Seems the current Velox code doesn't have the consistent white spaces definition? Could you check further?
And do we have some unit-tests to test CAST from VARCHAR in CI?
Is it possible to confirm it by all the tests in CI?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1798743977
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
Have you checked if they have the same sementic between vanilla Spark and velox?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406717233
@Henry2SS, thanks for your pr! This piece of scala code is for making Gluten/Velox consistent with Spark. Seems the current Velox code doesn't have the consistent white spaces definition? Could you check further?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406676322
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2431057091
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "Yohahaha (via GitHub)" <gi...@apache.org>.
Yohahaha commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1810511881
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
> Maybe add a config, default false, for it? A little worried about the result diff if we haven't tested the total case between vanilla spark and velox.
@Henry2SS would you?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "xumingming (via GitHub)" <gi...@apache.org>.
xumingming commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2408790137
> The performance is 1.x times faster.
@Henry2SS Do you mind show the concrete performance improvement?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1810503101
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
Maybe add a config, default false, for it? A little worried about the result diff if we haven't tested the total case between vanilla spark and velox.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406676031
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICEGJ-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on Github Issues?
https://github.com/apache/incubator-gluten/issues
Then could you also rename ***commit message*** and ***pull request title*** in the following format?
[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}
See also:
* [Other pull requests](https://github.com/apache/incubator-gluten/pulls/)
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1810555859
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
After looking at the velox code, the conversion from string to int/long is basically similar between [vanilla Spark](https://github.com/apache/spark/blob/master/common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java#L200) and [velox](https://github.com/facebookincubator/velox/blob/main/velox/functions/lib/string/StringImpl.h#L354). But for the conversion from string to float/double, [vanilla Spark](https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala#L390) uses `String.trim` to remove characters smaller than spaces, which is not same as velox, so we still need this extra trim.
A new config, which is default false, is good to me. Thanks @Henry2SS for working on it.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1810517585
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
> > Maybe add a config, default false, for it? A little worried about the result diff if we haven't tested the total case between vanilla spark and velox.
>
> @Henry2SS would you?
I'll do it later.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1814150195
##########
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala:
##########
@@ -2125,4 +2127,12 @@ object GlutenConfig {
"Otherwise, throw an exception.")
.booleanConf
.createWithDefault(true)
+
+ val CAST_FROM_VARCHAR_ADD_TRIM_NODE =
+ buildConf("spark.gluten.velox.castFromVarcharAddTrimNode")
+ .internal()
+ .doc("If true, will add a trim node which has the same sementic as vanilla Spark." +
+ "Otherwise, do nothing.")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
> need add this config to
I'll do this.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1812008029
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -730,20 +730,28 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
case FloatType | DoubleType | _: DecimalType =>
c.child.dataType match {
case StringType =>
Review Comment:
```suggestion
case StringType if GlutenConfig.getConf.castFromVarcharAddTrimNode =>
val trimNode = StringTrim(c.child, Some(Literal(trimSpaceStr)))
c.withNewChildren(Seq(trimNode)).asInstanceOf[Cast]
```
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -730,20 +730,28 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
case FloatType | DoubleType | _: DecimalType =>
c.child.dataType match {
case StringType =>
- val trimNode = StringTrim(c.child, Some(Literal(trimSpaceStr)))
- c.withNewChildren(Seq(trimNode)).asInstanceOf[Cast]
+ if (GlutenConfig.getConf.castFromVarcharAddTrimNode) {
+ val trimNode = StringTrim(c.child, Some(Literal(trimSpaceStr)))
+ c.withNewChildren(Seq(trimNode)).asInstanceOf[Cast]
+ } else {
+ c
+ }
case _ =>
c
}
case _ =>
c.child.dataType match {
case StringType =>
Review Comment:
```suggestion
case StringType if GlutenConfig.getConf.castFromVarcharAddTrimNode =>
val trimNode = StringTrim(
c.child,
Some(
Literal(trimWhitespaceStr +
trimSpaceSepStr + trimLineSepStr + trimParaSepStr)))
c.withNewChildren(Seq(trimNode)).asInstanceOf[Cast]
```
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] Add a config to control whether add trim node when CAST from varchar [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2434239855
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] Add a config to control whether add trim node when CAST from varchar [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2434174622
@Henry2SS, one code style violation.
```
11:10:48 error file=/home/jenkins/agent/workspace/gluten/gluten-ci/ut-stage-5/shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala message=File line length exceeds 100 characters line=2134
```
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] Add a config to control whether add trim node when CAST from varchar [incubator-gluten]
Posted by "Yohahaha (via GitHub)" <gi...@apache.org>.
Yohahaha commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2434285317
```
12:35:41 [ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.27.2:check (spotless-check) on project spark-sql-columnar-shims-common: The following files had format violations:
12:35:41 [ERROR] src/main/scala/org/apache/gluten/GlutenConfig.scala
12:35:41 [ERROR] @@ -311,7 +311,7 @@
12:35:41 [ERROR]
12:35:41 [ERROR] ··def·veloxBloomFilterMaxNumBits:·Long·=·conf.getConf(COLUMNAR_VELOX_BLOOM_FILTER_MAX_NUM_BITS)
12:35:41 [ERROR]
12:35:41 [ERROR] -··def·castFromVarcharAddTrimNode·:·Boolean·=·conf.getConf(CAST_FROM_VARCHAR_ADD_TRIM_NODE)
12:35:41 [ERROR] +··def·castFromVarcharAddTrimNode:·Boolean·=·conf.getConf(CAST_FROM_VARCHAR_ADD_TRIM_NODE)
12:35:41 [ERROR]
12:35:41 [ERROR] ··case·class·ResizeRange(min:·Int,·max:·Int)·{
12:35:41 [ERROR] ····assert(max·>=·min)
12:35:41 [ERROR] @@ -2131,9 +2131,10 @@
12:35:41 [ERROR] ··val·CAST_FROM_VARCHAR_ADD_TRIM_NODE·=
12:35:41 [ERROR] ····buildConf("spark.gluten.velox.castFromVarcharAddTrimNode")
12:35:41 [ERROR] ······.internal()
12:35:41 [ERROR] -······.doc("If·true,·will·add·a·trim·node·"·+
12:35:41 [ERROR] -········"which·has·the·same·sementic·as·vanilla·Spark·to·CAST-from-varchar."·+
12:35:41 [ERROR] -········"Otherwise,·do·nothing.")
12:35:41 [ERROR] +······.doc(
12:35:41 [ERROR] +········"If·true,·will·add·a·trim·node·"·+
12:35:41 [ERROR] +··········"which·has·the·same·sementic·as·vanilla·Spark·to·CAST-from-varchar."·+
12:35:41 [ERROR] +··········"Otherwise,·do·nothing.")
12:35:41 [ERROR] ······.booleanConf
12:35:41 [ERROR] ······.createWithDefault(false)
12:35:41 [ERROR] }
12:35:41 [ERROR] Run 'mvn spotless:apply' to fix these violations.
```
CH CI failed due to style check, try use `mvn spotless:apply -P backends-clickhouse -DskipTests` to fix.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] Add a config to control whether add trim node when CAST from varchar [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2434385237
> ```
> 12:35:41 [ERROR] Run 'mvn spotless:apply' to fix these violations.
> ```
>
> CH CI failed due to style check, try use `mvn spotless:apply -P backends-clickhouse -DskipTests` to fix.
done
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] Add a config to control whether add trim node when CAST from varchar [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2434384868
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1814138367
##########
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala:
##########
@@ -2125,4 +2127,12 @@ object GlutenConfig {
"Otherwise, throw an exception.")
.booleanConf
.createWithDefault(true)
+
+ val CAST_FROM_VARCHAR_ADD_TRIM_NODE =
+ buildConf("spark.gluten.velox.castFromVarcharAddTrimNode")
+ .internal()
+ .doc("If true, will add a trim node which has the same sementic as vanilla Spark." +
+ "Otherwise, do nothing.")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
It's ok to me. And if it's set default to false, we need add this config to `Configuration.md` to make sure users know about it.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2428847533
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406715054
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406736471
> @Henry2SS, thanks for your pr! This piece of scala code is for making Gluten/Velox consistent with Spark. Seems the current Velox code doesn't have the consistent white spaces definition? Could you check further?
There is a method called `isUnicodeWhiteSpace` in `velox/functions/lib/string/StringImpl.h`.
And I noticed that, from https://github.com/facebookincubator/velox/pull/7377 this PR, the author proposed different corner cases for Presto and Spark on CAST, including removeWhiteSpaces.
Unfortunately, I don't have much knowledge about this, please confirm.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406676863
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7476][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2406677545
https://github.com/apache/incubator-gluten/issues/7476
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2409877070
> > The performance is 1.x times faster.
>
> @Henry2SS Do you mind share the concrete performance improvement?
My pleasure.
There is a benchmark testing CAST VARCHAR to BIGINT. The sql is like `INSERT INTO tmp_table as select CAST(string_col as BIGINT) from table`. And the table has 2,879,966,589 rows.
We used gperftool to find that there is a function that takes up the most of the execution time in benchmark.
Before removing trim node, the total time of `ProjectExecTransformer` is 17.3 min ,dividing into 515 tasks.
After removing trim node, the total time of `ProjectExecTransformer` is 7.5 min ,dividing into 515 tasks.
So it is more than 1.x times faster.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1811667012
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -706,50 +706,6 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
VeloxGetStructFieldTransformer(substraitExprName, childTransformer, ordinal, original)
}
- /**
- * To align with spark in casting string type input to other types, add trim node for trimming
- * space or whitespace. See spark's Cast.scala.
- */
- override def genCastWithNewChild(c: Cast): Cast = {
- // scalastyle:off nonascii
- // Common whitespace to be trimmed, including: ' ', '\n', '\r', '\f', etc.
- val trimWhitespaceStr = " \t\n\u000B\u000C\u000D\u001C\u001D\u001E\u001F"
- // Space separator.
- val trimSpaceSepStr = "\u1680\u2008\u2009\u200A\u205F\u3000" +
- ('\u2000' to '\u2006').toList.mkString
- // Line separator.
- val trimLineSepStr = "\u2028"
Review Comment:
@Henry2SS, Spark UT doesn't cover many corner cases used in production workload. I'm afraid users would complain result mismatching issues due to this removement. Agree with @jackylee-ch, it's good to use a config to give the control to users. And maybe at some future time point, we can remove it.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: remove unnecessary trim function in CAST, cuz velox does it [incubator-gluten]
Posted by "Yohahaha (via GitHub)" <gi...@apache.org>.
Yohahaha commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2428936451
We can merge this PR when GHA passed and no blocking comments.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1812023608
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -730,20 +730,28 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
case FloatType | DoubleType | _: DecimalType =>
c.child.dataType match {
case StringType =>
- val trimNode = StringTrim(c.child, Some(Literal(trimSpaceStr)))
- c.withNewChildren(Seq(trimNode)).asInstanceOf[Cast]
+ if (GlutenConfig.getConf.castFromVarcharAddTrimNode) {
+ val trimNode = StringTrim(c.child, Some(Literal(trimSpaceStr)))
+ c.withNewChildren(Seq(trimNode)).asInstanceOf[Cast]
+ } else {
+ c
+ }
case _ =>
c
}
case _ =>
c.child.dataType match {
case StringType =>
Review Comment:
done
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -730,20 +730,28 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi {
case FloatType | DoubleType | _: DecimalType =>
c.child.dataType match {
case StringType =>
Review Comment:
done
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "Yohahaha (via GitHub)" <gi...@apache.org>.
Yohahaha commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1814130997
##########
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala:
##########
@@ -2125,4 +2127,12 @@ object GlutenConfig {
"Otherwise, throw an exception.")
.booleanConf
.createWithDefault(true)
+
+ val CAST_FROM_VARCHAR_ADD_TRIM_NODE =
+ buildConf("spark.gluten.velox.castFromVarcharAddTrimNode")
+ .internal()
+ .doc("If true, will add a trim node which has the same sementic as vanilla Spark." +
+ "Otherwise, do nothing.")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
I think we can set default value to false, because all GHA has passed and performance boost can not be ignored. If incorrect result issue occurs, we can fix it or make this config more fine-grained.
Just like scan parquet complex types, we remove force fallback config after all UT passed.
@PHILO-HE @jackylee-ch
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "Henry2SS (via GitHub)" <gi...@apache.org>.
Henry2SS commented on code in PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#discussion_r1814152251
##########
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala:
##########
@@ -2125,4 +2127,12 @@ object GlutenConfig {
"Otherwise, throw an exception.")
.booleanConf
.createWithDefault(true)
+
+ val CAST_FROM_VARCHAR_ADD_TRIM_NODE =
+ buildConf("spark.gluten.velox.castFromVarcharAddTrimNode")
+ .internal()
+ .doc("If true, will add a trim node which has the same sementic as vanilla Spark." +
+ "Otherwise, do nothing.")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
> It's ok to me. And if it's set default to false, we need add this config to `Configuration.md` to make sure users know about it.
I'll do this.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-7475][VL] fix: add a config to control whether a trim node is added in CAST [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7476:
URL: https://github.com/apache/incubator-gluten/pull/7476#issuecomment-2434129388
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org