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 2022/01/16 12:12:58 UTC

[GitHub] [spark] wangyum opened a new pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

wangyum opened a new pull request #35220:
URL: https://github.com/apache/spark/pull/35220


   ### What changes were proposed in this pull request?
   
   This pr improve `SimplifyCasts` to remove useless cast. For example:
   
   ```scala
   spark.sql("CREATE TABLE t1 (id int) using parquet")
   spark.sql("SELECT CAST(CAST(id AS DECIMAL(10, 0)) AS DECIMAL(12, 2)) FROM t1").explain(true)
   ```
   Before this pr:
   ```
   == Optimized Logical Plan ==
   Project [cast(cast(id#1 as decimal(10,0)) as decimal(12,2)) AS casted#0]
   +- Relation default.t1[id#1] parquet
   ```
   
   After this pr:
   ```
   == Optimized Logical Plan ==
   Project [cast(id#1 as decimal(12,2)) AS casted#0]
   +- Relation default.t1[id#1] parquet
   ```
   
   ### Why are the changes needed?
   
   Improve query performance.
   
   ### Does this PR introduce _any_ user-facing change?
   
   No.
   
   ### How was this patch tested?
   
   Unit test.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] wangyum commented on a change in pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
wangyum commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786756561



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -1037,6 +1037,9 @@ object SimplifyCasts extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
     _.containsPattern(CAST), ruleId) {
     case Cast(e, dataType, _, _) if e.dataType == dataType => e
+    case c1 @ Cast(Cast(e, d2: DecimalType, _, _), d1: DecimalType, _, _)
+        if d1.isWiderThan(d2) && d2.isWiderThan(e.dataType) =>

Review comment:
       OK




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] c21 commented on a change in pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
c21 commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r788134305



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -1037,13 +1037,26 @@ object SimplifyCasts extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
     _.containsPattern(CAST), ruleId) {
     case Cast(e, dataType, _, _) if e.dataType == dataType => e
+    case c @ Cast(Cast(e, dt1: NumericType, _, _), dt2: NumericType, _, _)
+        if isWiderCast(e.dataType, dt1) && isWiderCast(dt1, dt2) =>
+      c.copy(child = e)
     case c @ Cast(e, dataType, _, _) => (e.dataType, dataType) match {
       case (ArrayType(from, false), ArrayType(to, true)) if from == to => e
       case (MapType(fromKey, fromValue, false), MapType(toKey, toValue, true))
         if fromKey == toKey && fromValue == toValue => e
       case _ => c
       }
   }
+
+  // Returns whether the from DataType can be safely casted to the to DataType without losing
+  // any precision or range.
+  private def isWiderCast(from: DataType, to: DataType): Boolean = (from, to) match {

Review comment:
       It looks like `to` is always `NumericType` per caller in Line 1041. Do we want to define `to`'s type to be `NumericType` here?




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786423824



##########
File path: sql/core/src/test/resources/sql-tests/results/typeCoercion/native/concat.sql.out
##########
@@ -40,16 +40,16 @@ FROM (
 -- !query schema
 struct<col:string>
 -- !query output
-prefix_0120.0
-prefix_1231.0
-prefix_2342.0
-prefix_3453.0
-prefix_4564.0
-prefix_5675.0
-prefix_6786.0
-prefix_7897.0
-prefix_89108.0
-prefix_910119.0
+prefix_0120
+prefix_1231
+prefix_2342
+prefix_3453
+prefix_4564
+prefix_5675
+prefix_6786
+prefix_7897
+prefix_89108
+prefix_910119

Review comment:
       What causes this change? Maybe `Cast.canUpCast` is not strict enough.




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] c21 commented on a change in pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
c21 commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r788137705



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -1037,13 +1037,26 @@ object SimplifyCasts extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
     _.containsPattern(CAST), ruleId) {
     case Cast(e, dataType, _, _) if e.dataType == dataType => e
+    case c @ Cast(Cast(e, dt1: NumericType, _, _), dt2: NumericType, _, _)
+        if isWiderCast(e.dataType, dt1) && isWiderCast(dt1, dt2) =>
+      c.copy(child = e)
     case c @ Cast(e, dataType, _, _) => (e.dataType, dataType) match {
       case (ArrayType(from, false), ArrayType(to, true)) if from == to => e
       case (MapType(fromKey, fromValue, false), MapType(toKey, toValue, true))
         if fromKey == toKey && fromValue == toValue => e
       case _ => c
       }
   }
+
+  // Returns whether the from DataType can be safely casted to the to DataType without losing
+  // any precision or range.
+  private def isWiderCast(from: DataType, to: DataType): Boolean = (from, to) match {
+    case _ if from == to => true
+    case (from: NumericType, to: DecimalType) if to.isWiderThan(from) => true
+    case (from: DecimalType, to: NumericType) if from.isTighterThan(to) => true
+    case (from: IntegralType, to: IntegralType) => Cast.canUpCast(from, to)
+    case _ => false

Review comment:
       nit: why not combine Line 1054 and 1058 into:
   
   ```
   case _ => from == to
   ```




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] wangyum commented on pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
wangyum commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014112068


   cc @cloud-fan 


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan edited a comment on pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014594887


   @NobiGo that's why this PR only combines "upcast".  @wangyum I think we should call it out explicitly in the PR description. `remove useless cast` is too vague.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014216545


   Interesting. This simple change improves the plan for many TPC-DS queries, which is a low-hanging fruit that we have been missing for quite a while. Good catch!
   
   cc @sigmod 


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] wangyum commented on a change in pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
wangyum commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786765871



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -1037,13 +1037,26 @@ object SimplifyCasts extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
     _.containsPattern(CAST), ruleId) {
     case Cast(e, dataType, _, _) if e.dataType == dataType => e
+    case c @ Cast(Cast(e, dt1, _, _), dt2, _, _)
+        if isWiderCast(e.dataType, dt1) && isWiderCast(dt1, dt2) =>
+      c.copy(child = e)
     case c @ Cast(e, dataType, _, _) => (e.dataType, dataType) match {
       case (ArrayType(from, false), ArrayType(to, true)) if from == to => e
       case (MapType(fromKey, fromValue, false), MapType(toKey, toValue, true))
         if fromKey == toKey && fromValue == toValue => e
       case _ => c
       }
   }
+
+  // Returns whether the from DataType can be safely casted to the to DataType without losing
+  // any precision or range.
+  private def isWiderCast(from: DataType, to: DataType): Boolean = (from, to) match {
+    case _ if from == to => true
+    case (from: NumericType, to: DecimalType) if to.isWiderThan(from) => true
+    case (from: DecimalType, to: NumericType) if from.isTighterThan(to) => true
+    case (from: IntegralType, to: IntegralType) => Cast.canUpCast(from, to)
+    case _ => false
+  }

Review comment:
       We only support `NumericType`. not all data types can combine. For example:
   ```scala
   sql("create table t1(id timestamp) using parquet")
   sql("insert into t1 values(now())")
   sql("select cast(cast(id as long) as string) from t1").collect().foreach(println)
   ```




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] wangyum commented on a change in pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
wangyum commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786429857



##########
File path: sql/core/src/test/resources/sql-tests/results/typeCoercion/native/concat.sql.out
##########
@@ -40,16 +40,16 @@ FROM (
 -- !query schema
 struct<col:string>
 -- !query output
-prefix_0120.0
-prefix_1231.0
-prefix_2342.0
-prefix_3453.0
-prefix_4564.0
-prefix_5675.0
-prefix_6786.0
-prefix_7897.0
-prefix_89108.0
-prefix_910119.0
+prefix_0120
+prefix_1231
+prefix_2342
+prefix_3453
+prefix_4564
+prefix_5675
+prefix_6786
+prefix_7897
+prefix_89108
+prefix_910119

Review comment:
       It seems it is not safe when casting `IntegralType` to `DoubleType`.
   ```
   spark.sql("create table t1(id long) using parquet")
   spark.sql("insert into t1 values(1)")
   spark.sql("insert into t1 values(92233720368547758L)")
   spark.sql("select id, cast(cast(id as double) as string) from t1").show(false)
   ```
   Before this pr:
   ```
   +-----------------+----------------------------------+
   |id               |CAST(CAST(id AS DOUBLE) AS STRING)|
   +-----------------+----------------------------------+
   |92233720368547758|9.223372036854776E16              |
   |1                |1.0                               |
   +-----------------+----------------------------------+
   ```
   
   After this pr:
   ```
   +-----------------+----------------------------------+
   |id               |CAST(CAST(id AS DOUBLE) AS STRING)|
   +-----------------+----------------------------------+
   |1                |1                                 |
   |92233720368547758|92233720368547758                 |
   +-----------------+----------------------------------+
   ```




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] NobiGo commented on pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014426755


   In the Postgresql:
   ```
   CREATE TABLE decimalTable(value numeric(7,2));
   INSERT INTO decimalTable(value) VALUES (12.34);
   INSERT INTO decimalTable(value) VALUES (12.45);
   INSERT INTO decimalTable(value) VALUES (12.55);
   
   select value::decimal(5,1)::decimal(4,2) from decimalTable;
   ```
   ```
   value
   12.30
   12.50
   12.60
   ```
   
   If we simplify this cast to value::decimal(4,2) The value will be:
   
   ```
   value
   12.34
   12.45
   12.55
   ```
   
   The result will be different. So it is reasonable or acceptable in Spark?


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] NobiGo commented on pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014611159


   @cloud-fan Okay, I got it. Thanks.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] wangyum commented on a change in pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
wangyum commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786773563



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -1037,13 +1037,26 @@ object SimplifyCasts extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
     _.containsPattern(CAST), ruleId) {
     case Cast(e, dataType, _, _) if e.dataType == dataType => e
+    case c @ Cast(Cast(e, dt1, _, _), dt2, _, _)
+        if isWiderCast(e.dataType, dt1) && isWiderCast(dt1, dt2) =>
+      c.copy(child = e)
     case c @ Cast(e, dataType, _, _) => (e.dataType, dataType) match {
       case (ArrayType(from, false), ArrayType(to, true)) if from == to => e
       case (MapType(fromKey, fromValue, false), MapType(toKey, toValue, true))
         if fromKey == toKey && fromValue == toValue => e
       case _ => c
       }
   }
+
+  // Returns whether the from DataType can be safely casted to the to DataType without losing
+  // any precision or range.
+  private def isWiderCast(from: DataType, to: DataType): Boolean = (from, to) match {
+    case _ if from == to => true
+    case (from: NumericType, to: DecimalType) if to.isWiderThan(from) => true
+    case (from: DecimalType, to: NumericType) if from.isTighterThan(to) => true
+    case (from: IntegralType, to: IntegralType) => Cast.canUpCast(from, to)

Review comment:
       float to double is not needed because there are no data types can be safely cast to float or double. 




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014594887


   @NobiGo that's why this PR only combines "upcast"


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35220:
URL: https://github.com/apache/spark/pull/35220


   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] NobiGo commented on pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014604727


   @cloud-fan  I don't understand.  For example:
   ```
   select cast(cast(1234 as decimal(3,0)) as decimal(4,0)) from dual will throws exception
   select cast(1234 as decimal(4,0)) from dual will return the value
   ```


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014609396


   `cast(1234 as decimal(3,0))` this is not a "upcast", which should be 100% safe.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] wangyum commented on a change in pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
wangyum commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786351644



##########
File path: sql/core/src/test/resources/sql-tests/results/typeCoercion/native/concat.sql.out
##########
@@ -40,16 +40,16 @@ FROM (
 -- !query schema
 struct<col:string>
 -- !query output
-prefix_0120.0
-prefix_1231.0
-prefix_2342.0
-prefix_3453.0
-prefix_4564.0
-prefix_5675.0
-prefix_6786.0
-prefix_7897.0
-prefix_89108.0
-prefix_910119.0
+prefix_0120
+prefix_1231
+prefix_2342
+prefix_3453
+prefix_4564
+prefix_5675
+prefix_6786
+prefix_7897
+prefix_89108
+prefix_910119

Review comment:
       @cloud-fan Do you think it is safe?




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 pull request #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1017261948


   thanks, merging to master!


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786430773



##########
File path: sql/core/src/test/resources/sql-tests/results/typeCoercion/native/concat.sql.out
##########
@@ -40,16 +40,16 @@ FROM (
 -- !query schema
 struct<col:string>
 -- !query output
-prefix_0120.0
-prefix_1231.0
-prefix_2342.0
-prefix_3453.0
-prefix_4564.0
-prefix_5675.0
-prefix_6786.0
-prefix_7897.0
-prefix_89108.0
-prefix_910119.0
+prefix_0120
+prefix_1231
+prefix_2342
+prefix_3453
+prefix_4564
+prefix_5675
+prefix_6786
+prefix_7897
+prefix_89108
+prefix_910119

Review comment:
       yea, long -> double is not a 100% safe upcast because it may truncate. How about we define a `isWiderCast` in this rule and only put 100% safe upcast there?




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] wangyum commented on pull request #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
wangyum commented on pull request #35220:
URL: https://github.com/apache/spark/pull/35220#issuecomment-1014518831


   @NobiGo We do not simplify that case. This is the test: https://github.com/apache/spark/blob/c764303cd07adcb13acbd3269a9903bb6e86dccd/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyCastsSuite.scala#L88-L91


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #35220: [SPARK-37922][SQL] Combine to one cast if we can safely up-cast two casts

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r786762563



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -1037,13 +1037,26 @@ object SimplifyCasts extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
     _.containsPattern(CAST), ruleId) {
     case Cast(e, dataType, _, _) if e.dataType == dataType => e
+    case c @ Cast(Cast(e, dt1, _, _), dt2, _, _)
+        if isWiderCast(e.dataType, dt1) && isWiderCast(dt1, dt2) =>
+      c.copy(child = e)
     case c @ Cast(e, dataType, _, _) => (e.dataType, dataType) match {
       case (ArrayType(from, false), ArrayType(to, true)) if from == to => e
       case (MapType(fromKey, fromValue, false), MapType(toKey, toValue, true))
         if fromKey == toKey && fromValue == toValue => e
       case _ => c
       }
   }
+
+  // Returns whether the from DataType can be safely casted to the to DataType without losing
+  // any precision or range.
+  private def isWiderCast(from: DataType, to: DataType): Boolean = (from, to) match {
+    case _ if from == to => true
+    case (from: NumericType, to: DecimalType) if to.isWiderThan(from) => true
+    case (from: DecimalType, to: NumericType) if from.isTighterThan(to) => true
+    case (from: IntegralType, to: IntegralType) => Cast.canUpCast(from, to)

Review comment:
       how about float -> double?




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #35220: [SPARK-37922][SQL] Improve `SimplifyCasts` to remove useless cast

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35220:
URL: https://github.com/apache/spark/pull/35220#discussion_r785694477



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -1037,6 +1037,9 @@ object SimplifyCasts extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
     _.containsPattern(CAST), ruleId) {
     case Cast(e, dataType, _, _) if e.dataType == dataType => e
+    case c1 @ Cast(Cast(e, d2: DecimalType, _, _), d1: DecimalType, _, _)
+        if d1.isWiderThan(d2) && d2.isWiderThan(e.dataType) =>

Review comment:
       can we generalize it? I think the general idea is we can combine to "upcast" into one.




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org