[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations URL: https://github.com/apache/spark/pull/26995#discussion_r370438991 ## File path: sql/core/src/test/resources/sql-tests/results/interval.sql.out ## @@ -1101,3 +1102,45 @@ select interval '1 ' day struct -- !query 113 output 1 days + + +-- !query 114 +select -(a) from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 114 schema +struct<(- a):interval> +-- !query 114 output +-178956970 years -8 months + + +-- !query 115 +select a - b from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 115 schema +struct<(a - b):interval> +-- !query 115 output +1 months + + +-- !query 116 +select b + interval '1 month' from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 116 schema +struct<(b + INTERVAL '1 months'):interval> +-- !query 116 output +-178956970 years -8 months + + +-- !query 117 +select a * 1.1 from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 117 schema +struct<> +-- !query 117 output +java.lang.ArithmeticException +integer overflow Review comment: I am OK with your suggestion，but first let us also 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations URL: https://github.com/apache/spark/pull/26995#discussion_r370438991 ## File path: sql/core/src/test/resources/sql-tests/results/interval.sql.out ## @@ -1101,3 +1102,45 @@ select interval '1 ' day struct -- !query 113 output 1 days + + +-- !query 114 +select -(a) from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 114 schema +struct<(- a):interval> +-- !query 114 output +-178956970 years -8 months + + +-- !query 115 +select a - b from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 115 schema +struct<(a - b):interval> +-- !query 115 output +1 months + + +-- !query 116 +select b + interval '1 month' from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 116 schema +struct<(b + INTERVAL '1 months'):interval> +-- !query 116 output +-178956970 years -8 months + + +-- !query 117 +select a * 1.1 from values (interval '-2147483648 months', interval '2147483647 months') t(a, b) +-- !query 117 schema +struct<> +-- !query 117 output +java.lang.ArithmeticException +integer overflow Review comment: I am OK with your suggestion，but first let us also 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362406768
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala
##
@@ -197,10 +198,22 @@ class IntervalExpressionsSuite extends SparkFunSuite
with ExpressionEvalHelper {
}
test("multiply") {
-def check(interval: String, num: Double, expected: String): Unit = {
- checkEvaluation(
-MultiplyInterval(Literal(stringToInterval(interval)), Literal(num)),
-if (expected == null) null else stringToInterval(expected))
+def check(
+interval: String,
+num: Double,
+expected: String,
+checkException: Boolean = false): Unit = {
+ Seq("true", "false").foreach { v =>
+withSQLConf(SQLConf.ANSI_ENABLED.key -> v) {
+ if (checkException) {
+checkExceptionInExpression[ArithmeticException](
+ MultiplyInterval(Literal(stringToInterval(interval)),
Literal(num)), expected)
Review comment:
yes, we can also use `safeStringToInterval(expected) == null` to choose way
to check 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362399970
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
##
@@ -75,12 +75,20 @@ case class UnaryMinus(child: Expression) extends
UnaryExpression
"""})
case _: CalendarIntervalType =>
val iu = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
- defineCodeGen(ctx, ev, c => s"$iu.negate($c)")
Review comment:
fixed
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362399560
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##
@@ -110,49 +110,51 @@ object ExtractIntervalPart {
}
}
-abstract class IntervalNumOperation(
-interval: Expression,
-num: Expression,
-operation: (CalendarInterval, Double) => CalendarInterval,
-operationName: String)
+abstract class IntervalNumOperation(interval: Expression, num: Expression,
operationName: String)
extends BinaryExpression with ImplicitCastInputTypes with Serializable {
+
override def left: Expression = interval
override def right: Expression = num
override def inputTypes: Seq[AbstractDataType] = Seq(CalendarIntervalType,
DoubleType)
+
override def dataType: DataType = CalendarIntervalType
override def nullable: Boolean = true
- override def nullSafeEval(interval: Any, num: Any): Any = {
-try {
- operation(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
-} catch {
- case _: java.lang.ArithmeticException => null
-}
+ override def nullSafeEval(interval: Any, num: Any): Any = operationName
match {
+case "multiply" =>
+ multiplyExact(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+case "divide" =>
+ if (num == 0) return null
Review comment:
shall we fix numerics / 0 when ANSI is on too in a followup?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362391807
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##
@@ -110,49 +110,51 @@ object ExtractIntervalPart {
}
}
-abstract class IntervalNumOperation(
-interval: Expression,
-num: Expression,
-operation: (CalendarInterval, Double) => CalendarInterval,
-operationName: String)
+abstract class IntervalNumOperation(interval: Expression, num: Expression,
operationName: String)
extends BinaryExpression with ImplicitCastInputTypes with Serializable {
+
override def left: Expression = interval
override def right: Expression = num
override def inputTypes: Seq[AbstractDataType] = Seq(CalendarIntervalType,
DoubleType)
+
override def dataType: DataType = CalendarIntervalType
override def nullable: Boolean = true
- override def nullSafeEval(interval: Any, num: Any): Any = {
-try {
- operation(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
-} catch {
- case _: java.lang.ArithmeticException => null
-}
+ override def nullSafeEval(interval: Any, num: Any): Any = operationName
match {
+case "multiply" =>
+ multiplyExact(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+case "divide" =>
+ if (num == 0) return null
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362391445
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##
@@ -110,49 +110,51 @@ object ExtractIntervalPart {
}
}
-abstract class IntervalNumOperation(
-interval: Expression,
-num: Expression,
-operation: (CalendarInterval, Double) => CalendarInterval,
-operationName: String)
+abstract class IntervalNumOperation(interval: Expression, num: Expression,
operationName: String)
extends BinaryExpression with ImplicitCastInputTypes with Serializable {
+
override def left: Expression = interval
override def right: Expression = num
override def inputTypes: Seq[AbstractDataType] = Seq(CalendarIntervalType,
DoubleType)
+
override def dataType: DataType = CalendarIntervalType
override def nullable: Boolean = true
- override def nullSafeEval(interval: Any, num: Any): Any = {
-try {
- operation(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
-} catch {
- case _: java.lang.ArithmeticException => null
-}
+ override def nullSafeEval(interval: Any, num: Any): Any = operationName
match {
+case "multiply" =>
+ multiplyExact(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+case "divide" =>
+ if (num == 0) return null
Review comment:
https://github.com/apache/spark/blob/b6793816c16da1eea33dbb9d63e90a9b25394d45/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L362
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362389473
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##
@@ -110,49 +110,51 @@ object ExtractIntervalPart {
}
}
-abstract class IntervalNumOperation(
-interval: Expression,
-num: Expression,
-operation: (CalendarInterval, Double) => CalendarInterval,
-operationName: String)
+abstract class IntervalNumOperation(interval: Expression, num: Expression,
operationName: String)
extends BinaryExpression with ImplicitCastInputTypes with Serializable {
+
override def left: Expression = interval
override def right: Expression = num
override def inputTypes: Seq[AbstractDataType] = Seq(CalendarIntervalType,
DoubleType)
+
override def dataType: DataType = CalendarIntervalType
override def nullable: Boolean = true
- override def nullSafeEval(interval: Any, num: Any): Any = {
-try {
- operation(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
-} catch {
- case _: java.lang.ArithmeticException => null
-}
+ override def nullSafeEval(interval: Any, num: Any): Any = operationName
match {
+case "multiply" =>
+ multiplyExact(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+case "divide" =>
+ if (num == 0) return null
Review comment:
I did this because other numerics return null whether ansi or not
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362374933
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala
##
@@ -211,14 +222,17 @@ class IntervalExpressionsSuite extends SparkFunSuite
with ExpressionEvalHelper {
check("-100 years -1 millisecond", 0.5, "-50 years -500 microseconds")
check("2 months 4 seconds", -0.5, "-1 months -2 seconds")
check("1 month 2 microseconds", 1.5, "1 months 15 days 3 microseconds")
-check("2 months", Int.MaxValue, null)
+check("2 months", Int.MaxValue, CalendarInterval.MAX_VALUE.toString,
checkException = true)
}
test("divide") {
def check(interval: String, num: Double, expected: String): Unit = {
- checkEvaluation(
-DivideInterval(Literal(stringToInterval(interval)), Literal(num)),
-if (expected == null) null else stringToInterval(expected))
+ Seq("true", "false").foreach { v =>
+withSQLConf(SQLConf.ANSI_ENABLED.key -> v) {
+ checkEvaluation(DivideInterval(Literal(stringToInterval(interval)),
Literal(num)),
+if (expected == null) null else stringToInterval(expected))
Review comment:
yes, I will add a check 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r362135240
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##
@@ -110,50 +110,57 @@ object ExtractIntervalPart {
}
}
-abstract class IntervalNumOperation(
-interval: Expression,
-num: Expression,
-operation: (CalendarInterval, Double) => CalendarInterval,
-operationName: String)
+abstract class IntervalNumOperation(interval: Expression, num: Expression,
operationName: String)
extends BinaryExpression with ImplicitCastInputTypes with Serializable {
+
+ protected val methodStr: String =
+IntervalUtils.getClass.getName.stripSuffix("$") + "." + operationName +
"Exact"
+
override def left: Expression = interval
override def right: Expression = num
override def inputTypes: Seq[AbstractDataType] = Seq(CalendarIntervalType,
DoubleType)
+
override def dataType: DataType = CalendarIntervalType
override def nullable: Boolean = true
+ override def prettyName: String = operationName + "_interval"
+}
+
+case class MultiplyInterval(interval: Expression, num: Expression)
Review comment:
yes, it is
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361930716
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,6 +415,49 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
Review comment:
ok, make sense
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361675825
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,6 +415,49 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
Review comment:
`WHEN ANSI OFF`
1. If we use `NULL ON OVERFLOW` policy for all arithmetics, means interval
`add/subtract/negate` break their backward compatibility to fit
`multiply/divide`, seem not a good choice.
2. If we remain to use `NULL ON OVERFLOW` policy for `multiply/divide` and
java overflow style for `add `etc, this breaks no backward compatibility but
also keeps the inconsistency. seem not a good choice either.
3. If we throw exceptions for `multiply/divide` for overflows and remain
java overflow style for `add` etc, this breaks no backward compatibility but
also raises much worse inconsistency issue.
4. If we throw exceptions for all interval arithmetics, this is just `ANSI
ON`.
5. Behavior of current implementation, we do not break backward
compatibility either. Things only go "wrong" only when it reaches the
`CalendarInterval.MAX_VALUE` or `CalendarInterval.MIN_VALUE`. It seems the best
choice among the worst.
Things became odd since we didn't support fraction representations for
intervals, but support `multiply/divide` intervals with fractions. a.k.a we do
not have consistent rules https://github.com/apache/spark/pull/26592 for how
the fraction part of month go days, how day fraction go milliseconds.
There seems no other systems can refer to for our `ANSI OFF` mode.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361675825
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,6 +415,49 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
Review comment:
`WHEN ANSI OFF`
There seems no
1. If we use `NULL ON OVERFLOW` policy for all arithmetics, means interval
`add/subtract/negate` break their backward compatibility to fit
`multiply/divide`, seem not a good choice.
2. If we remain to use `NULL ON OVERFLOW` policy for `multiply/divide` and
java overflow style for `add `etc, this breaks no backward compatibility but
also keeps the inconsistency. seem not a good choice either.
3. If we throw exceptions for `multiply/divide` for overflows and remain
java overflow style for `add` etc, this breaks no backward compatibility but
also raises much worse inconsistency issue.
4. If we throw exceptions for all interval arithmetics, this is just `ANSI
ON`.
5. Behavior of current implementation, we do not break backward
compatibility either. Things only go wrong when it reaches the
`CalendarInterval.MAX_VALUE`. It seems the best choice among the worst.
Things became odd since we didn't support fraction representations for
intervals, but support `multiply/divide` intervals with fractions. a.k.a we do
not have consistent rules https://github.com/apache/spark/pull/26592 for how
the fraction part of month go days, how day fraction go milliseconds.
There seems no other systems can refer to for our `ANSI OFF` mode.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361675825
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,6 +415,49 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
Review comment:
`WHEN ANSI OFF`
1. If we use `NULL ON OVERFLOW` policy for all arithmetics, means interval
`add/subtract/negate` break their backward compatibility to fit
`multiply/divide`, seem not a good choice.
2. If we remain to use `NULL ON OVERFLOW` policy for `multiply/divide` and
java overflow style for `add `etc, this breaks no backward compatibility but
also keeps the inconsistency. seem not a good choice either.
3. If we throw exceptions for `multiply/divide` for overflows and remain
java overflow style for `add` etc, this breaks no backward compatibility but
also raises much worse inconsistency issue.
4. If we throw exceptions for all interval arithmetics, this is just `ANSI
ON`.
5. Behavior of current implementation, we do not break backward
compatibility either. Things only go wrong when it reaches the
`CalendarInterval.MAX_VALUE`. It seems the best choice among the worst.
Things became odd since we didn't support fraction representations for
intervals, but support `multiply/divide` intervals with fractions. a.k.a we do
not have consistent rules https://github.com/apache/spark/pull/26592 for how
the fraction part of month go days, how day fraction go milliseconds.
There seems no other systems can refer to for our `ANSI OFF` mode.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361656171
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,6 +415,49 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
Review comment:
the behavior with current implementation follows `select
cast(9223372036854775807L * 1.1D as Long);` results 9223372036854775807L
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361651024
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,6 +415,49 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
Review comment:
hmm, then for multiply/divide overflow we return `null`s for other
arithmetics we return overflowed values when ansi off? this seems weird 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361651024
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,6 +415,49 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
Review comment:
hmm, then for multiply/divide overflow we return `null`s for other
arithmetics we return overflowed values? this seems weird 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361649973
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala
##
@@ -112,49 +113,96 @@ object ExtractIntervalPart {
}
}
-abstract class IntervalNumOperation(
-interval: Expression,
-num: Expression,
-operation: (CalendarInterval, Double) => CalendarInterval,
-operationName: String)
+abstract class IntervalNumOperation(interval: Expression, num: Expression)
extends BinaryExpression with ImplicitCastInputTypes with Serializable {
+
+ protected val checkOverflow: Boolean = SQLConf.get.ansiEnabled
+
override def left: Expression = interval
override def right: Expression = num
override def inputTypes: Seq[AbstractDataType] = Seq(CalendarIntervalType,
DoubleType)
override def dataType: DataType = CalendarIntervalType
override def nullable: Boolean = true
+}
+
+case class MultiplyInterval(interval: Expression, num: Expression)
+ extends IntervalNumOperation(interval, num) {
+
+ override def prettyName: String = "multiply_interval"
override def nullSafeEval(interval: Any, num: Any): Any = {
try {
- operation(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+ if (checkOverflow) {
+multiplyExact(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+ } else {
+multiply(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+ }
} catch {
- case _: java.lang.ArithmeticException => null
+ case _: ArithmeticException if !checkOverflow => null
}
}
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
nullSafeCodeGen(ctx, ev, (interval, num) => {
val iu = IntervalUtils.getClass.getName.stripSuffix("$")
+ val operationName = if (checkOverflow) "multiplyExact" else "multiply"
s"""
try {
${ev.value} = $iu.$operationName($interval, $num);
-} catch (java.lang.ArithmeticException e) {
- ${ev.isNull} = true;
+} catch (ArithmeticException e) {
+ if ($checkOverflow) {
+throw e;
+ } else {
+${ev.isNull} = true;
+ }
}
"""
})
}
-
- override def prettyName: String = operationName + "_interval"
}
-case class MultiplyInterval(interval: Expression, num: Expression)
- extends IntervalNumOperation(interval, num, multiply, "multiply")
-
case class DivideInterval(interval: Expression, num: Expression)
- extends IntervalNumOperation(interval, num, divide, "divide")
+ extends IntervalNumOperation(interval, num) {
+
+ override def prettyName: String = "divide_interval"
+
+ override def nullSafeEval(interval: Any, num: Any): Any = {
+try {
+ if (num == 0) return null
+ if (checkOverflow) {
+divideExact(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+ } else {
+divide(interval.asInstanceOf[CalendarInterval],
num.asInstanceOf[Double])
+ }
+} catch {
+ case _: ArithmeticException if !checkOverflow => null
Review comment:
ahh, I c..
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361596662
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
##
@@ -413,45 +415,141 @@ object IntervalUtils {
new CalendarInterval(truncatedMonths, truncatedDays, micros.round)
}
+ /**
+ * Makes an interval from months, days and micros with the fractional part by
+ * adding the month fraction to days and the days fraction to micros.
+ */
+ private def safeFromDoubles(
+ monthsWithFraction: Double,
+ daysWithFraction: Double,
+ microsWithFraction: Double): CalendarInterval = {
+val monthInLong = monthsWithFraction.toLong
+val truncatedMonths = if (monthInLong > Int.MaxValue) {
+ Int.MaxValue
+} else if (monthInLong < Int.MinValue) {
+ Int.MinValue
+} else {
+ monthInLong.toInt
+}
+val days = daysWithFraction + DAYS_PER_MONTH * (monthsWithFraction -
truncatedMonths)
+val dayInLong = days.toLong
+val truncatedDays = if (dayInLong > Int.MaxValue) {
+ Int.MaxValue
+} else if (monthInLong < Int.MinValue) {
+ Int.MinValue
+} else {
+ dayInLong.toInt
+}
+val micros = microsWithFraction + MICROS_PER_DAY * (days - truncatedDays)
+new CalendarInterval(truncatedMonths, truncatedDays.toInt, micros.round)
+ }
+
/**
* Unary minus, return the negated the calendar interval value.
*
* @param interval the interval to be negated
* @return a new calendar interval instance with all it parameters negated
from the origin one.
+ * @throws ArithmeticException if the result overflows any field value
*/
def negate(interval: CalendarInterval): CalendarInterval = {
+val months = Math.negateExact(interval.months)
Review comment:
sgtm
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361146102
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
##
@@ -218,6 +252,11 @@ object BinaryArithmetic {
""")
case class Add(left: Expression, right: Expression) extends BinaryArithmetic {
+ override def nullable: Boolean = dataType match {
+case CalendarIntervalType if !checkOverflow => true
Review comment:
changed behavior to a) ANSI on: exception, b) ANSI off: java style overflow
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361096801
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
##
@@ -218,6 +252,11 @@ object BinaryArithmetic {
""")
case class Add(left: Expression, right: Expression) extends BinaryArithmetic {
+ override def nullable: Boolean = dataType match {
+case CalendarIntervalType if !checkOverflow => true
Review comment:
```
spark-sql> select cast('128' as tinyint);
NULL
Time taken: 0.029 seconds, Fetched 1 row(s)
spark-sql> select cast(128 as tinyint);
-128
```
not quite related to this pr, the cast logic seems unconsisent 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361093504
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
##
@@ -218,6 +252,11 @@ object BinaryArithmetic {
""")
case class Add(left: Expression, right: Expression) extends BinaryArithmetic {
+ override def nullable: Boolean = dataType match {
+case CalendarIntervalType if !checkOverflow => true
Review comment:
Yes, we have.
we now have `+/-/unaray_-` in java overflow behavior(2.4 or maybe earlier)
and we have '*/-' in null for overflow behavior (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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361089663
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
##
@@ -75,12 +80,29 @@ case class UnaryMinus(child: Expression) extends
UnaryExpression
"""})
case _: CalendarIntervalType =>
val iu = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
- defineCodeGen(ctx, ev, c => s"$iu.negate($c)")
+ nullSafeCodeGen(ctx, ev, interval => s"""
+try {
+ ${ev.value} = $iu.negate($interval);
Review comment:
Seems a safeNegate need a patch for generated code, like
```java
s"""
| if (${ev.value} = null) {
|(${ev.isNull} = true;
|}
"""
```, which does not make thing better
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361088588
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
##
@@ -218,6 +252,11 @@ object BinaryArithmetic {
""")
case class Add(left: Expression, right: Expression) extends BinaryArithmetic {
+ override def nullable: Boolean = dataType match {
+case CalendarIntervalType if !checkOverflow => true
Review comment:
Yes, the current behavior of master is separating a) decimal(which is null
for overflow) from b) other types. This pr (so far)is just adding intervals to
group a). We may reach an agreement on which way to follow first.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL] Overflow check for interval arithmetic operations
yaooqinn commented on a change in pull request #26995: [SPARK-30341][SQL]
Overflow check for interval arithmetic operations
URL: https://github.com/apache/spark/pull/26995#discussion_r361085310
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
##
@@ -75,12 +80,29 @@ case class UnaryMinus(child: Expression) extends
UnaryExpression
"""})
case _: CalendarIntervalType =>
val iu = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
- defineCodeGen(ctx, ev, c => s"$iu.negate($c)")
+ nullSafeCodeGen(ctx, ev, interval => s"""
+try {
+ ${ev.value} = $iu.negate($interval);
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
