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/18 21:40:11 UTC

[GitHub] [spark] MaxGekk opened a new pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

MaxGekk opened a new pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628
 
 
   ### What changes were proposed in this pull request?
   In the PR, I propose to add the `returnLong` parameter to `IntegralDivide`, and pass the value of `spark.sql.legacy.integralDivide.returnBigint` if `returnLong` is not provided on creation of `IntegralDivide`.
   
   ### 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.
   
   OptionsAttachments
   
   ### Does this PR introduce any user-facing change?
   No
   
   ### How was this patch tested?
   By `ArithmeticExpressionSuite`.

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587899184
 
 
   **[Test build #118654 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118654/testReport)** for PR 27628 at commit [`ebeec38`](https://github.com/apache/spark/commit/ebeec388f5b6f2a1b3b3cb5e972a7262ee463aef).

----------------------------------------------------------------
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 a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381090086
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -403,11 +403,18 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
   """,
   since = "3.0.0")
 // scalastyle:on line.size.limit
-case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+case class IntegralDivide(
+    left: Expression,
+    right: Expression,
+    returnLong: Boolean) extends DivModLike {
 
 Review comment:
   can we just add a `private val returnLong = SQLConf.get.integralDivideReturnLong` in the class body? Then the config value is fixed when the expression is created. And it can be serialized to executors.
   
   The spark Expression constructor is kind of exposed to end users when they call functions in SQL. BTW `Cast` already use a `val` to store config values.
   

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587900969
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23406/
   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] MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381408085
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -403,11 +403,18 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
   """,
   since = "3.0.0")
 // scalastyle:on line.size.limit
-case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+case class IntegralDivide(
+    left: Expression,
+    right: Expression,
+    returnLong: Boolean) extends DivModLike {
 
 Review comment:
   Yes, we count only expressions, see https://github.com/apache/spark/blob/919d551ddbf7575abe7fe47d4bbba62164d6d845/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L601-L602

----------------------------------------------------------------
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 closed pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628
 
 
   

----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381411021
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -451,6 +458,12 @@ case class IntegralDivide(left: Expression, right: Expression) extends DivModLik
   override def evalOperation(left: Any, right: Any): Any = div(left, right)
 }
 
+object IntegralDivide {
+  def apply(left: Expression, right: Expression): IntegralDivide = {
+    new IntegralDivide(left, right)
+  }
+}
 
 Review comment:
   If I define `unapply`, it will be not used. Not sure that is is needed.

----------------------------------------------------------------
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 a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381276518
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -403,11 +403,18 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
   """,
   since = "3.0.0")
 // scalastyle:on line.size.limit
-case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+case class IntegralDivide(
+    left: Expression,
+    right: Expression,
+    returnLong: Boolean) extends DivModLike {
 
 Review comment:
   so the non-expression parameter doesn't count? Then I'm fine with 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.
 
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] MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381411021
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -451,6 +458,12 @@ case class IntegralDivide(left: Expression, right: Expression) extends DivModLik
   override def evalOperation(left: Any, right: Any): Any = div(left, right)
 }
 
+object IntegralDivide {
+  def apply(left: Expression, right: Expression): IntegralDivide = {
+    new IntegralDivide(left, right)
+  }
+}
 
 Review comment:
   If I define `unapply`, it will be no used. Not sure that is is needed.

----------------------------------------------------------------
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] MaxGekk commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-588494366
 
 
   @hvanhovell Some of them, others (except of Json/CSV exprs) influence defaultElementType

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587995183
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118654/
   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] hvanhovell commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
hvanhovell commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381246285
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -403,11 +403,18 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
   """,
   since = "3.0.0")
 // scalastyle:on line.size.limit
-case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+case class IntegralDivide(
+    left: Expression,
+    right: Expression,
+    returnLong: Boolean) extends DivModLike {
 
 Review comment:
   That can potentially change value every time you transform the tree.

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-589021736
 
 
   thanks, merging to master/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 removed a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587995180
 
 
   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] MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381275275
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -403,11 +403,18 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
   """,
   since = "3.0.0")
 // scalastyle:on line.size.limit
-case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+case class IntegralDivide(
+    left: Expression,
+    right: Expression,
+    returnLong: Boolean) extends DivModLike {
 
 Review comment:
   > I'm just worried about we allow end users to specify the returnLong parameter which becomes an API.
   
   We don't allow that:
   ```sql
   SELECT div(3, 2, false);
   ```
   fails with:
   ```
   Invalid number of arguments for function div. Expected: 2; Found: 3; line 1 pos 7
   org.apache.spark.sql.AnalysisException: Invalid number of arguments for function div. Expected: 2; Found: 3; line 1 pos 7
   	at org.apache.spark.sql.catalyst.analysis.FunctionRegistry$.$anonfun$expression$7(FunctionRegistry.scala:618)
   	at scala.Option.getOrElse(Option.scala:189)
   	at org.apache.spark.sql.catalyst.analysis.FunctionRegistry$.$anonfun$expression$4(FunctionRegistry.scala:602)
   	at org.apache.spark.sql.catalyst.analysis.SimpleFunctionRegistry.lookupFunction(FunctionRegistry.scala:121)
   	at org.apache.spark.sql.catalyst.catalog.SessionCatalog.lookupFunction(SessionCatalog.scala:1418)
   ```
   I ran the command on the PR changes.

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587899184
 
 
   **[Test build #118654 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118654/testReport)** for PR 27628 at commit [`ebeec38`](https://github.com/apache/spark/commit/ebeec388f5b6f2a1b3b3cb5e972a7262ee463aef).

----------------------------------------------------------------
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] MaxGekk edited a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk edited a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-588438082
 
 
   There are at least a few more places:
   1. https://github.com/apache/spark/blob/ab186e36597e16d48f8eb16d57e7a39b6b67d4e4/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L95
   2. https://github.com/apache/spark/blob/4e50f0291f032b4a5c0b46ed01fdef14e4cbb050/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/csvExpressions.scala#L96
   3. https://github.com/apache/spark/blob/8e280cebf25e47bf40df224461a76fc4c84cc997/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala#L562
   4. https://github.com/apache/spark/blob/b0db6231fddefbd418da3309bc76f54bc7875549/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala#L528-L529
   5. https://github.com/apache/spark/blob/b917a6593dc969b9b766259eb8cbbd6e90e0dc53/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala#L50
   6. https://github.com/apache/spark/blob/926e3a1efe9e142804fcbf52146b22700640ae1b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L49
   7. https://github.com/apache/spark/blob/926e3a1efe9e142804fcbf52146b22700640ae1b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L149

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587900969
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/23406/
   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 a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381277017
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -451,6 +458,12 @@ case class IntegralDivide(left: Expression, right: Expression) extends DivModLik
   override def evalOperation(left: Any, right: Any): Any = div(left, right)
 }
 
+object IntegralDivide {
+  def apply(left: Expression, right: Expression): IntegralDivide = {
+    new IntegralDivide(left, right)
+  }
+}
 
 Review comment:
   shall we define `unapply` as well? Most of the time we don't care about the `returnLong` paramter. When we do, we should write `case e @ IntegralDivide(left, right) if e.returnLong`.

----------------------------------------------------------------
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 a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381248387
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -403,11 +403,18 @@ case class Divide(left: Expression, right: Expression) extends DivModLike {
   """,
   since = "3.0.0")
 // scalastyle:on line.size.limit
-case class IntegralDivide(left: Expression, right: Expression) extends DivModLike {
+case class IntegralDivide(
+    left: Expression,
+    right: Expression,
+    returnLong: Boolean) extends DivModLike {
 
 Review comment:
   or how about we create 2 expressions `IntegralDivide` and `IntegralDivideReturnLong`? I'm just worried about we allow end users to specify the `returnLong` parameter which becomes an API.

----------------------------------------------------------------
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] MaxGekk commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-588438082
 
 
   There are at least a few more places:
   1. https://github.com/apache/spark/blob/ab186e36597e16d48f8eb16d57e7a39b6b67d4e4/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L95
   2. https://github.com/apache/spark/blob/4e50f0291f032b4a5c0b46ed01fdef14e4cbb050/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/csvExpressions.scala#L96
   3. https://github.com/apache/spark/blob/8e280cebf25e47bf40df224461a76fc4c84cc997/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala#L562
   4. https://github.com/apache/spark/blob/b0db6231fddefbd418da3309bc76f54bc7875549/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala#L528-L529
   5. https://github.com/apache/spark/blob/b917a6593dc969b9b766259eb8cbbd6e90e0dc53/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala#L50
   6. https://github.com/apache/spark/blob/926e3a1efe9e142804fcbf52146b22700640ae1b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L49
   7. https://github.com/apache/spark/blob/926e3a1efe9e142804fcbf52146b22700640ae1b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L149
   8.  

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587900942
 
 
   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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-589010345
 
 
   @MaxGekk thanks for composing the list! I've created an umbrella JIRA for them: https://issues.apache.org/jira/browse/SPARK-30893

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587994723
 
 
   **[Test build #118654 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118654/testReport)** for PR 27628 at commit [`ebeec38`](https://github.com/apache/spark/commit/ebeec388f5b6f2a1b3b3cb5e972a7262ee463aef).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `case class IntegralDivide(`

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587900942
 
 
   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] MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381411021
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -451,6 +458,12 @@ case class IntegralDivide(left: Expression, right: Expression) extends DivModLik
   override def evalOperation(left: Any, right: Any): Any = div(left, right)
 }
 
+object IntegralDivide {
+  def apply(left: Expression, right: Expression): IntegralDivide = {
+    new IntegralDivide(left, right)
+  }
+}
 
 Review comment:
   If I define `unapply`, it will be nor used. Not sure that is is needed.

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-588219518
 
 
   @MaxGekk not related to this PR, can you check other places that call `SQLConf.get` inside expressions? We should try to make them consistent. One exception is `Cast`, we decided to create `AnsiCast` expression.

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587995180
 
 
   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] MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#discussion_r381409851
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
 ##########
 @@ -451,6 +458,12 @@ case class IntegralDivide(left: Expression, right: Expression) extends DivModLik
   override def evalOperation(left: Any, right: Any): Any = div(left, right)
 }
 
+object IntegralDivide {
+  def apply(left: Expression, right: Expression): IntegralDivide = {
+    new IntegralDivide(left, right)
+  }
+}
 
 Review comment:
   There is only one place where we unapply `IntegralDivide`. It is here https://github.com/apache/spark/pull/27628/files#diff-8e1575bb706d6f7e8b5ea0b175eaeafcR178 . And `returnLong` is not checked, it is just extracted and copy-pasted.

----------------------------------------------------------------
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 #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-587995183
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118654/
   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] hvanhovell commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes

Posted by GitBox <gi...@apache.org>.
hvanhovell commented on issue #27628: [SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes
URL: https://github.com/apache/spark/pull/27628#issuecomment-588474990
 
 
   @MaxGekk do all of them influence the dataType or the nullability? 

----------------------------------------------------------------
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