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

MaxGekk · 2022-06-13T17:06:28Z

What changes were proposed in this pull request?

In the PR, I propose to support casts of ANSI intervals to decimals, and follow the SQL standard:

Why are the changes needed?

To improve user experience with Spark SQL, and to conform to the SQL standard.

Does this PR introduce any user-facing change?

No, it just extends existing behavior of casts.

Before:

spark-sql> SELECT CAST(INTERVAL '1.001002' SECOND AS DECIMAL(10, 6));
Error in query: cannot resolve 'CAST(INTERVAL '01.001002' SECOND AS DECIMAL(10,6))' due to data type mismatch: cannot cast interval second to decimal(10,6); line 1 pos 7;
'Project [unresolvedalias(cast(INTERVAL '01.001002' SECOND as decimal(10,6)), None)]
+- OneRowRelation

After:

spark-sql> SELECT CAST(INTERVAL '1.001002' SECOND AS DECIMAL(10, 6));
1.001002

How was this patch tested?

By running new tests:

$ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z cast.sql"
$ build/sbt "test:testOnly *CastWithAnsiOnSuite"
$ build/sbt "test:testOnly *CastWithAnsiOffSuite"

MaxGekk · 2022-06-14T18:46:05Z

cc @srielau

MaxGekk · 2022-06-15T08:34:00Z

@cloud-fan @gengliangwang @HyukjinKwon @srielau Could you review this PR, please.

cloud-fan · 2022-06-15T08:38:47Z

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

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


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

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.

yea let's add some negative tests

Added a test

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.

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

I plan to do that.

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.

…-to-decimal

srielau · 2022-06-15T15:04:20Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala

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


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

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.

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.

cloud-fan · 2022-06-15T15:31:22Z

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

        case _: NumberFormatException => null
      }
+    case x: DayTimeIntervalType =>
+      buildCast[Long](_, dt => changePrecision(dayTimeIntervalToDecimal(dt, x.endField), target))


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

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

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

let's add a new parameter then.

I have added new parameter already.

cloud-fan · 2022-06-16T01:42:57Z

sql/core/src/test/resources/sql-tests/inputs/cast.sql

+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));
+
+select cast(interval '10.123' second as decimal(1, 0));


how about decimal(4, 2)? will we fail or truncate?

I added a check. The result is truncated. Please, take a look.

MaxGekk · 2022-06-16T05:14:36Z

@cloud-fan @srielau I added more checks, please, take a look at this PR one more time.

cloud-fan · 2022-06-16T06:23:31Z

sql/core/src/test/resources/sql-tests/results/ansi/cast.sql.out

+-- !query schema
+struct<CAST(INTERVAL '10.005' SECOND AS DECIMAL(4,2)):decimal(4,2)>
+-- !query output
+10.01


@srielau I think the rounding behavior is correct.

MaxGekk · 2022-06-16T09:06:14Z

Merging to master. Thank you, @cloud-fan and @srielau for review.

### What changes were proposed in this pull request? In the PR, I propose to update the doc page https://spark.apache.org/docs/latest/sql-ref-ansi-compliance.html#cast about new behaviour of CAST introduced by the following PRs: 1. [[SPARK-40008][SQL] Support casting of integrals to ANSI intervals](#37442) 2. [[SPARK-39451][SQL] Support casting intervals to integrals in ANSI mode](#36811) 3. [[SPARK-39470][SQL] Support cast of ANSI intervals to decimals](#36857) 4. [[SPARK-40014][SQL] Support cast of decimals to ANSI intervals](#37466) <img width="1004" alt="Screenshot 2022-08-12 at 15 00 10" src="https://user-images.githubusercontent.com/1580697/184349887-700c64f6-e9c8-40c4-85cc-a77072506cf5.png"> ### Why are the changes needed? To inform Spark SQL about new behaviour of `CAST`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By building Spark SQL docs and check them manually: ``` $ SKIP_SCALADOC=1 SKIP_PYTHONDOC=1 SKIP_RDOC=1 bundle exec jekyll build ``` Closes #37495 from MaxGekk/doc-interval-cast. Authored-by: Max Gekk <[email protected]> Signed-off-by: Max Gekk <[email protected]>

MaxGekk added 2 commits June 13, 2022 19:01

Support cast of an ANSI interval to a decimal

ade9160

Fix issues

d992ca5

github-actions bot added the SQL label Jun 13, 2022

MaxGekk added 3 commits June 14, 2022 20:08

Fix a bug

3160dd5

Add tests

679571d

Add random tests

d6fc899

MaxGekk changed the title ~~[WIP][SQL] Support cast of ANSI intervals to decimals~~ [WIP][SPARK-39470][SQL] Support cast of ANSI intervals to decimals Jun 14, 2022

MaxGekk changed the title ~~[WIP][SPARK-39470][SQL] Support cast of ANSI intervals to decimals~~ [SPARK-39470][SQL] Support cast of ANSI intervals to decimals Jun 14, 2022

MaxGekk marked this pull request as ready for review June 14, 2022 18:45

MaxGekk requested review from HyukjinKwon, cloud-fan and gengliangwang June 14, 2022 18:45

cloud-fan reviewed Jun 15, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into cast-ansi-intervals…

77818ff

…-to-decimal

srielau reviewed Jun 15, 2022

View reviewed changes

cloud-fan reviewed Jun 15, 2022

View reviewed changes

Pass nullOnOverflow

90fb367

cloud-fan reviewed Jun 16, 2022

View reviewed changes

Add more checks

654b2d3

cloud-fan reviewed Jun 16, 2022

View reviewed changes

cloud-fan approved these changes Jun 16, 2022

View reviewed changes

MaxGekk closed this in d2332d9 Jun 16, 2022

MaxGekk mentioned this pull request Aug 12, 2022

[SPARK-40061][SQL][DOCS] Document cast of ANSI intervals #37495

Closed

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

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

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

Uh oh!

Conversation

MaxGekk commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

MaxGekk commented Jun 14, 2022

Uh oh!

MaxGekk commented Jun 15, 2022

Uh oh!

cloud-fan Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srielau Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jun 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jun 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MaxGekk commented Jun 13, 2022 •

edited

Loading

cloud-fan Jun 15, 2022 •

edited

Loading

MaxGekk Jun 15, 2022 •

edited

Loading

srielau Jun 15, 2022 •

edited

Loading

MaxGekk Jun 15, 2022 •

edited

Loading