You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/02/20 19:07:22 UTC
[GitHub] [spark] peter-toth opened a new pull request #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
peter-toth opened a new pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656
### What changes were proposed in this pull request?
This PR adds a new `nullOnOverflow` parameter to `MakeDecimal` so as to avoid its value depending on `SQLConf.get` and change during planning.
### Why are the changes needed?
This allows to avoid the issue when the configuration change between different phases of planning, and this can silently break a query plan which can lead to crashes or data corruption.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
Existing UTs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589895938
cc @hvanhovell
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27656: [SPARK-30898][SQL] The
behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590294774
This changes the nullability, so we should have it in 3.0. We'd better highlight it in PR description.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27656: [SPARK-30898][SQL] The
behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589257819
**[Test build #118731 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118731/testReport)** for PR 27656 at commit [`c1c1259`](https://github.com/apache/spark/commit/c1c1259f5daf0b9937636c112161941493fdad3b).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on issue #27656: [SPARK-30898][SQL] The
behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590502902
Btw, we should better have a test for this too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] peter-toth commented on a change in pull request #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r383468170
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
In my understanding what we want to avoid is that `nullOnOverflow` changes during expression transformation: https://github.com/apache/spark/pull/27683#issuecomment-590317541
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589258469
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23482/
Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589416303
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118731/
Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] peter-toth commented on a change in pull request #27656:
[SPARK-30898][SQL] The nullability of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #27656: [SPARK-30898][SQL] The nullability of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r384324015
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
All these 8 in https://issues.apache.org/jira/browse/SPARK-30893 should be tested? I can try to add some kind of test in a follow-up PR next week.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r384242299
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
Yea, better to add tests if we can.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589416278
Merged build finished. Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on issue #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589257819
**[Test build #118731 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118731/testReport)** for PR 27656 at commit [`c1c1259`](https://github.com/apache/spark/commit/c1c1259f5daf0b9937636c112161941493fdad3b).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589416278
Merged build finished. Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r383462042
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
Is this change different to previous one?
Original `nullOnOverflow` is a `val` that is initialized during constructing of `MakeDecimal` and won't be changed then, no?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on issue #27656: [SPARK-30898][SQL] The
behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590553179
@cloud-fan Have we reached a consensus about fixed behavior regarding SQL.get? If so, I think we should have a clear note in migration guide.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589416303
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118731/
Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on issue #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589258459
Merged build finished. Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27656: [SPARK-30898][SQL] The
behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590264908
Note: the major reason to do this is to avoid changing nullability at runtime. It's still under discussion that if we want to make the behavior fixed after the expression was created.
cc @viirya @maropu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589258459
Merged build finished. Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #27656:
[SPARK-30898][SQL] The nullability of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #27656: [SPARK-30898][SQL] The nullability of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r384327767
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
Yea, I think we should do where possible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on issue #27656: [SPARK-30898][SQL] The
nullability of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
maropu commented on issue #27656: [SPARK-30898][SQL] The nullability of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-591295951
At least, I think the new expressions we will add in future should follow this design (the expressions should not depend on `SQLConf` directly). So, is it worth documenting it somewhere for novice developers?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r383518204
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
Oh, I see. Makes sense now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on issue #27656: [SPARK-30898][SQL] The
behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589413675
**[Test build #118731 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118731/testReport)** for PR 27656 at commit [`c1c1259`](https://github.com/apache/spark/commit/c1c1259f5daf0b9937636c112161941493fdad3b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class MakeDecimal(`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r383462042
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
Is this change different to previous one?
Original `nullOnOverflow` is a `val` that is initialized during constructing of `MakeDecimal` and won't be changed then, isn't?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590291374
We can revert it from 3.0. Please go ahead if you feel so @cloud-fan. I don't mind.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590651393
Yes, probably we should have test cases for these.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #27656:
[SPARK-30898][SQL] The behavior of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r383519155
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
Can we add a test for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590200098
Merged to master and branch-3.0.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on issue #27656: [SPARK-30898][SQL]
The behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-589258469
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23482/
Test PASSed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on issue #27656: [SPARK-30898][SQL] The
behavior of MakeDecimal should not depend on SQLConf.get
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27656: [SPARK-30898][SQL] The behavior of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#issuecomment-590667607
> Have we reached a consensus about fixed behavior regarding SQL.get?
I just realized that it's already fixed... The expressions put the result of `SQLConf.get.blabla` in a `val` so it's fixed after the expression is created. But this can be changed unexpectedly when we transform the expression tree with different SQL configs. I don't know how it affects real-world queries but can be a potential issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #27656:
[SPARK-30898][SQL] The nullability of MakeDecimal should not depend on
SQLConf.get
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #27656: [SPARK-30898][SQL] The nullability of MakeDecimal should not depend on SQLConf.get
URL: https://github.com/apache/spark/pull/27656#discussion_r384329641
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -45,9 +45,15 @@ case class UnscaledValue(child: Expression) extends UnaryExpression {
* Note: this expression is internal and created only by the optimizer,
* we don't need to do type check for it.
*/
-case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression {
+case class MakeDecimal(
+ child: Expression,
+ precision: Int,
+ scale: Int,
+ nullOnOverflow: Boolean) extends UnaryExpression {
- private val nullOnOverflow = !SQLConf.get.ansiEnabled
Review comment:
We should have tests to for such behaviors. Otherwise, we could possibly change it unintentionally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org