Skip to content

[SPARK-39470][SQL] Support cast of ANSI intervals to decimals #36857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGekk wants to merge 8 commits into from
Closed

[SPARK-39470][SQL] Support cast of ANSI intervals to decimals #36857

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ade9160
Support cast of an ANSI interval to a decimal
MaxGekk Jun 13, 2022
d992ca5
Fix issues
MaxGekk Jun 13, 2022
3160dd5
Fix a bug
MaxGekk Jun 14, 2022
679571d
Add tests
MaxGekk Jun 14, 2022
d6fc899
Add random tests
MaxGekk Jun 14, 2022
77818ff
Merge remote-tracking branch 'origin/master' into cast-ansi-intervals…
MaxGekk Jun 15, 2022
90fb367
Pass nullOnOverflow
MaxGekk Jun 15, 2022
654b2d3
Add more checks
MaxGekk Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import org.apache.spark.sql.catalyst.util._
import org.apache.spark.sql.catalyst.util.DateTimeConstants._
import org.apache.spark.sql.catalyst.util.DateTimeUtils._
import org.apache.spark.sql.catalyst.util.IntervalStringStyles.ANSI_STYLE
import org.apache.spark.sql.catalyst.util.IntervalUtils.{dayTimeIntervalToByte, dayTimeIntervalToInt, dayTimeIntervalToLong, dayTimeIntervalToShort, yearMonthIntervalToByte, yearMonthIntervalToInt, yearMonthIntervalToShort}
import org.apache.spark.sql.catalyst.util.IntervalUtils.{dayTimeIntervalToByte, dayTimeIntervalToDecimal, dayTimeIntervalToInt, dayTimeIntervalToLong, dayTimeIntervalToShort, yearMonthIntervalToByte, yearMonthIntervalToInt, yearMonthIntervalToShort}
import org.apache.spark.sql.errors.QueryExecutionErrors
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._
Expand Down Expand Up @@ -110,7 +110,7 @@ object Cast {
case (StringType, _: CalendarIntervalType) => true
case (StringType, _: AnsiIntervalType) => true

case (_: AnsiIntervalType, _: IntegralType) => true
case (_: AnsiIntervalType, _: IntegralType | _: DecimalType) => true

case (_: DayTimeIntervalType, _: DayTimeIntervalType) => true
case (_: YearMonthIntervalType, _: YearMonthIntervalType) => true
Expand Down Expand Up @@ -194,8 +194,7 @@ object Cast {

case (_: DayTimeIntervalType, _: DayTimeIntervalType) => true
case (_: YearMonthIntervalType, _: YearMonthIntervalType) => true
case (_: DayTimeIntervalType, _: IntegralType) => true
case (_: YearMonthIntervalType, _: IntegralType) => true
case (_: AnsiIntervalType, _: IntegralType | _: DecimalType) => true

case (StringType, _: NumericType) => true
case (BooleanType, _: NumericType) => true
Expand Down Expand Up @@ -1015,6 +1014,11 @@ case class Cast(
} catch {
case _: NumberFormatException => null
}
case x: DayTimeIntervalType =>
buildCast[Long](_, dt => changePrecision(dayTimeIntervalToDecimal(dt, x.endField), target))

@cloud-fan cloud-fan Jun 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaxGekk as you said I think it makes sense to always go with ANSI behavior in new features. Does changePrecision take a bool flag like nullOnOverflow?

@MaxGekk MaxGekk Jun 15, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does changePrecision take a bool flag like nullOnOverflow?

No, it checks the SQL config actually, passed to the Cast expression:

  private[this] def changePrecision(value: Decimal, decimalType: DecimalType): Decimal = {
    if (value.changePrecision(decimalType.precision, decimalType.scale)) {
      value
    } else {
      if (!ansiEnabled) {
        null
      } else {
        throw QueryExecutionErrors.cannotChangeDecimalPrecisionError(
          value, decimalType.precision, decimalType.scale, queryContext)
      }
    }
  }

Probably, it makes sense to incapsulate the logic:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

Lines 993 to 1000 in a4f96af

case StringType if !ansiEnabled =>
buildCast[UTF8String](_, s => {
val d = Decimal.fromString(s)
if (d == null) null else changePrecision(d, target)
})
case StringType if ansiEnabled =>
buildCast[UTF8String](_,
s => changePrecision(Decimal.fromStringANSI(s, target, queryContext), target))

Also cc @gengliangwang

@gengliangwang gengliangwang Jun 15, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah always go with ANSI behavior in new features. Either adding a new parameter on changePrecision or adding a new method are fine.

@cloud-fan cloud-fan Jun 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add a new parameter then.

@MaxGekk MaxGekk Jun 16, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added new parameter already.

case x: YearMonthIntervalType =>
buildCast[Int](_, ym =>
changePrecision(Decimal(yearMonthIntervalToInt(ym, x.startField, x.endField)), target))

@cloud-fan cloud-fan Jun 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we consider ansi mode here? what if the decimal precision is too small, e.g. decimal(1, 0)?

@MaxGekk MaxGekk Jun 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we did before, we don't consider non-ANSI mode for ANSI intervals. So, we are in ANSI mode always for ANSI intervals.

what if the decimal precision is too small, e.g. decimal(1, 0)

I think changePrecision() should output an error. Let me double check this.

@cloud-fan cloud-fan Jun 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea let's add some negative tests

@MaxGekk MaxGekk Jun 15, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test

@srielau srielau Jun 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take confusion on what is scale and what is precision to my grave....

We can round/truncate, but not modulo/wrap.
So I see:
CAST(INTERVAL '1.234' SECOND AS DECIMAL(1,0)) as perfectly fine.
But:
CAST(INTERVAL('123.4' SECOND AS DECIMAL(4, 2)) is an overflow.
On trunc vs round the semantic is given in DECIMAL to DECIMAL:
select cast(1.5 AS DECIMAL(1, 0));
2

PS: I loathe that cannot-change-decimal-precision error. In most cases we should just call it an overflow.

@MaxGekk MaxGekk Jun 15, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: I loathe that cannot-change-decimal-precision error. In most cases we should just call it an overflow.

I plan to do that.

@MaxGekk MaxGekk Jun 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, changing of scale/precision while keeping the same value is different thing from changing the value and get overflow.

In any case, I am not going to change decimal logic (errors) here in the PR.

}

// DoubleConverter
Expand Down Expand Up @@ -1605,6 +1609,22 @@ case class Cast(
$evNull = true;
}
"""
case x: DayTimeIntervalType =>
(c, evPrim, evNull) =>
val u = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
code"""
Decimal $tmp = $u.dayTimeIntervalToDecimal($c, (byte)${x.endField});
${changePrecision(tmp, target, evPrim, evNull, canNullSafeCast, ctx)}
"""
case x: YearMonthIntervalType =>
(c, evPrim, evNull) =>
val u = IntervalUtils.getClass.getCanonicalName.stripSuffix("$")
val tmpYm = ctx.freshVariable("tmpYm", classOf[Int])
code"""
int $tmpYm = $u.yearMonthIntervalToInt($c, (byte)${x.startField}, (byte)${x.endField});
Decimal $tmp = Decimal.apply($tmpYm);
${changePrecision(tmp, target, evPrim, evNull, canNullSafeCast, ctx)}
"""
}
}

Expand Down
9 changes: 9 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1346,6 +1346,15 @@ object IntervalUtils {
}
}

def dayTimeIntervalToDecimal(v: Long, endField: Byte): Decimal = {
endField match {
case DAY => Decimal(v / MICROS_PER_DAY)
case HOUR => Decimal(v / MICROS_PER_HOUR)
case MINUTE => Decimal(v / MICROS_PER_MINUTE)
case SECOND => Decimal(v, Decimal.MAX_LONG_DIGITS, 6)
}
}

def dayTimeIntervalToInt(v: Long, startField: Byte, endField: Byte): Int = {
val vLong = dayTimeIntervalToLong(v, startField, endField)
val vInt = vLong.toInt
Expand Down
33 changes: 33 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,4 +1272,37 @@ abstract class CastSuiteBase extends SparkFunSuite with ExpressionEvalHelper {
"to restore the behavior before Spark 3.0."))
}
}

test("cast ANSI intervals to decimals") {
Seq(

@srielau srielau Jun 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need examples with rounding:
INTERVAL '12.123' SECOND AS DECIMAL(3, 1) => 12.1
INTERVAL '12.005' SECOND AS DECIMAL(4, 2) => 12.01

@MaxGekk MaxGekk Jun 15, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't round and don't loose info. See the last check that I added:

select cast(interval '10.123' second as decimal(1, 0))
-- !query schema
struct<>
-- !query output
org.apache.spark.SparkArithmeticException
[CANNOT_CHANGE_DECIMAL_PRECISION] Decimal(compact, 10, 18, 6) cannot be represented as Decimal(1, 0). If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error.
== SQL(line 1, position 8) ==
select cast(interval '10.123' second as decimal(1, 0))
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Both of your cases are the same, actually - total number of digits is greater than 3 or 4.

@srielau srielau Jun 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is different. My examples test sucess. Yours tests a failure. Obviously any number>= 10 and <100 cannot be even approximated in a single digit.
But any number between 0 and 9 can be approximated in a digit.
I’m curious why you see truncation rather than rounding given that decimal to decimal rounds.

(Duration.ZERO, DayTimeIntervalType(DAY), DecimalType(10, 3)) -> Decimal(0, 10, 3),
(Duration.ofHours(-1), DayTimeIntervalType(HOUR), DecimalType(10, 1)) -> Decimal(-10, 10, 1),
(Duration.ofMinutes(1), DayTimeIntervalType(MINUTE), DecimalType(8, 2)) -> Decimal(100, 8, 2),
(Duration.ofSeconds(59), DayTimeIntervalType(SECOND), DecimalType(6, 0)) -> Decimal(59, 6, 0),
(Duration.ofSeconds(-60).minusMillis(1), DayTimeIntervalType(SECOND),
DecimalType(10, 3)) -> Decimal(-60.001, 10, 3),
(Duration.ZERO, DayTimeIntervalType(DAY, SECOND), DecimalType(10, 6)) -> Decimal(0, 10, 6),
(Duration.ofHours(-23).minusMinutes(59).minusSeconds(59).minusNanos(123456000),
DayTimeIntervalType(HOUR, SECOND), DecimalType(18, 6)) -> Decimal(-86399.123456, 18, 6),
(Period.ZERO, YearMonthIntervalType(YEAR), DecimalType(5, 2)) -> Decimal(0, 5, 2),
(Period.ofMonths(-1), YearMonthIntervalType(MONTH),
DecimalType(8, 0)) -> Decimal(-1, 8, 0),
(Period.ofYears(-1).minusMonths(1), YearMonthIntervalType(YEAR, MONTH),
DecimalType(8, 3)) -> Decimal(-13000, 8, 3)
).foreach { case ((duration, intervalType, targetType), expected) =>
checkEvaluation(
Cast(Literal.create(duration, intervalType), targetType),
expected)
}

dayTimeIntervalTypes.foreach { it =>
checkConsistencyBetweenInterpretedAndCodegen((child: Expression) =>
Cast(child, DecimalType.USER_DEFAULT), it)
}

yearMonthIntervalTypes.foreach { it =>
checkConsistencyBetweenInterpretedAndCodegen((child: Expression) =>
Cast(child, DecimalType.USER_DEFAULT), it)
}
}
}
6 changes: 6 additions & 0 deletions sql/core/src/test/resources/sql-tests/inputs/cast.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,9 @@ select cast(interval '10' day as bigint);

select cast(interval '-1000' month as tinyint);
select cast(interval '1000000' second as smallint);

-- cast ANSI intervals to decimals
select cast(interval '-1' year as decimal(10, 0));
select cast(interval '1.000001' second as decimal(10, 6));
select cast(interval '08:11:10.001' hour to second as decimal(10, 4));
select cast(interval '1 01:02:03.1' day to second as decimal(8, 1));
34 changes: 33 additions & 1 deletion sql/core/src/test/resources/sql-tests/results/ansi/cast.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
-- Automatically generated by SQLQueryTestSuite
-- Number of queries: 81
-- Number of queries: 85


-- !query
Expand Down Expand Up @@ -841,3 +841,35 @@ struct<>
-- !query output
org.apache.spark.SparkArithmeticException
[CAST_OVERFLOW] The value INTERVAL '1000000' SECOND of the type "INTERVAL SECOND" cannot be cast to "SMALLINT" due to an overflow. Use `try_cast` to tolerate overflow and return NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error.


-- !query
select cast(interval '-1' year as decimal(10, 0))
-- !query schema
struct<CAST(INTERVAL '-1' YEAR AS DECIMAL(10,0)):decimal(10,0)>
-- !query output
-1


-- !query
select cast(interval '1.000001' second as decimal(10, 6))
-- !query schema
struct<CAST(INTERVAL '01.000001' SECOND AS DECIMAL(10,6)):decimal(10,6)>
-- !query output
1.000001


-- !query
select cast(interval '08:11:10.001' hour to second as decimal(10, 4))
-- !query schema
struct<CAST(INTERVAL '08:11:10.001' HOUR TO SECOND AS DECIMAL(10,4)):decimal(10,4)>
-- !query output
29470.0010


-- !query
select cast(interval '1 01:02:03.1' day to second as decimal(8, 1))
-- !query schema
struct<CAST(INTERVAL '1 01:02:03.1' DAY TO SECOND AS DECIMAL(8,1)):decimal(8,1)>
-- !query output
90123.1
Expand Down
34 changes: 33 additions & 1 deletion sql/core/src/test/resources/sql-tests/results/cast.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
-- Automatically generated by SQLQueryTestSuite
-- Number of queries: 81
-- Number of queries: 85


-- !query
Expand Down Expand Up @@ -669,3 +669,35 @@ struct<>
-- !query output
org.apache.spark.SparkArithmeticException
[CAST_OVERFLOW] The value INTERVAL '1000000' SECOND of the type "INTERVAL SECOND" cannot be cast to "SMALLINT" due to an overflow. Use `try_cast` to tolerate overflow and return NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error.


-- !query
select cast(interval '-1' year as decimal(10, 0))
-- !query schema
struct<CAST(INTERVAL '-1' YEAR AS DECIMAL(10,0)):decimal(10,0)>
-- !query output
-1


-- !query
select cast(interval '1.000001' second as decimal(10, 6))
-- !query schema
struct<CAST(INTERVAL '01.000001' SECOND AS DECIMAL(10,6)):decimal(10,6)>
-- !query output
1.000001


-- !query
select cast(interval '08:11:10.001' hour to second as decimal(10, 4))
-- !query schema
struct<CAST(INTERVAL '08:11:10.001' HOUR TO SECOND AS DECIMAL(10,4)):decimal(10,4)>
-- !query output
29470.0010


-- !query
select cast(interval '1 01:02:03.1' day to second as decimal(8, 1))
-- !query schema
struct<CAST(INTERVAL '1 01:02:03.1' DAY TO SECOND AS DECIMAL(8,1)):decimal(8,1)>
-- !query output
90123.1
Expand Down