-
Notifications
You must be signed in to change notification settings - Fork 342
feat: support interval types and make_ym_interval / make_dt_interval #4541
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.
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
Changes from all commits
d30105b
c48a950
d8f51b9
2d3ffd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,7 @@ object Utils extends CometTypeShim with Logging { | |
| case yi: ArrowType.Interval if yi.getUnit == IntervalUnit.YEAR_MONTH => | ||
| YearMonthIntervalType() | ||
| case di: ArrowType.Interval if di.getUnit == IntervalUnit.DAY_TIME => DayTimeIntervalType() | ||
| case d: ArrowType.Duration if d.getUnit == TimeUnit.MICROSECOND => DayTimeIntervalType() | ||
| case t: ArrowType.Time if t.getUnit == TimeUnit.NANOSECOND && t.getBitWidth == 64 => | ||
| // scalastyle:off classforname | ||
| val clazz = Class.forName("org.apache.spark.sql.types.TimeType$") | ||
|
|
@@ -152,6 +153,10 @@ object Utils extends CometTypeShim with Logging { | |
| case NullType => ArrowType.Null.INSTANCE | ||
| case dt if isTimeType(dt) => | ||
| new ArrowType.Time(TimeUnit.NANOSECOND, 64) | ||
| case _: YearMonthIntervalType => new ArrowType.Interval(IntervalUnit.YEAR_MONTH) | ||
| // Spark stores DayTimeIntervalType as microseconds in an int64, matching Arrow | ||
| // Duration(Microsecond) rather than the lossy Interval(DayTime) {days, millis} layout. | ||
| case _: DayTimeIntervalType => new ArrowType.Duration(TimeUnit.MICROSECOND) | ||
|
Comment on lines
+156
to
+159
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified correct, no change needed for this PR. The representation choices match Spark's internal storage exactly: One forward-looking note, not a blocker. Both mappings drop Spark's interval field qualifiers. |
||
| case _ => | ||
| throw new UnsupportedOperationException( | ||
| s"Unsupported data type: [${dt.getClass.getName}] ${dt.catalogString}") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| -- Licensed to the Apache Software Foundation (ASF) under one | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The coverage of column, literal, default, negative, and null inputs is good, and using One gap worth filling: overflow. query expect_error(ARITHMETIC_OVERFLOW)
SELECT make_ym_interval(178956971, 0)Please confirm the exact error token the suite matches. The value
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @mbutrovich. I added overflow tests |
||
| -- or more contributor license agreements. See the NOTICE file | ||
| -- distributed with this work for additional information | ||
| -- regarding copyright ownership. The ASF licenses this file | ||
| -- to you under the Apache License, Version 2.0 (the | ||
| -- "License"); you may not use this file except in compliance | ||
| -- with the License. You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, | ||
| -- software distributed under the License is distributed on an | ||
| -- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| -- KIND, either express or implied. See the License for the | ||
| -- specific language governing permissions and limitations | ||
| -- under the License. | ||
|
|
||
| -- Routes make_dt_interval through the codegen dispatcher; produces DayTimeIntervalType. | ||
|
|
||
| statement | ||
| CREATE TABLE test_mdi(d int, h int, mi int, s decimal(18,6)) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO test_mdi VALUES (1, 2, 3, 4.5), (0, 0, 0, 0), (-1, 0, 30, 15.250), (NULL, 1, 1, 1) | ||
|
|
||
| query | ||
| SELECT d, h, mi, s, make_dt_interval(d, h, mi, s) FROM test_mdi | ||
|
|
||
| -- literal arguments | ||
| query | ||
| SELECT make_dt_interval(1, 2, 3, 4.5), make_dt_interval(0, 0, 0, 0) | ||
|
|
||
| -- default arguments | ||
| query | ||
| SELECT make_dt_interval(1), make_dt_interval(1, 2), make_dt_interval() | ||
|
|
||
| -- overflow: days * MICROS_PER_DAY exceeds the int64 microsecond range. makeDayTimeInterval throws | ||
| -- unconditionally (not ANSI-gated); this confirms the dispatched codegen path propagates Spark's | ||
| -- exception. The pattern is the lowercase word so it matches every version: Spark 4.x raises | ||
| -- INTERVAL_ARITHMETIC_OVERFLOW ("Integer overflow while operating with intervals") while Spark 3.x | ||
| -- raises a raw ArithmeticException ("long overflow"). | ||
| query expect_error(overflow) | ||
| SELECT make_dt_interval(2147483647) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| -- Licensed to the Apache Software Foundation (ASF) under one | ||
| -- or more contributor license agreements. See the NOTICE file | ||
| -- distributed with this work for additional information | ||
| -- regarding copyright ownership. The ASF licenses this file | ||
| -- to you under the Apache License, Version 2.0 (the | ||
| -- "License"); you may not use this file except in compliance | ||
| -- with the License. You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, | ||
| -- software distributed under the License is distributed on an | ||
| -- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| -- KIND, either express or implied. See the License for the | ||
| -- specific language governing permissions and limitations | ||
| -- under the License. | ||
|
|
||
| -- Routes make_ym_interval through the codegen dispatcher; produces YearMonthIntervalType. | ||
|
|
||
| statement | ||
| CREATE TABLE test_myi(y int, m int) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO test_myi VALUES (1, 2), (0, 0), (-1, 6), (5, -3), (NULL, 3), (2, NULL) | ||
|
|
||
| query | ||
| SELECT y, m, make_ym_interval(y, m) FROM test_myi | ||
|
|
||
| -- literal arguments | ||
| query | ||
| SELECT make_ym_interval(1, 2), make_ym_interval(0, 0), make_ym_interval(-5, 11) | ||
|
|
||
| -- default arguments | ||
| query | ||
| SELECT make_ym_interval(3), make_ym_interval() | ||
|
|
||
| -- overflow: years * 12 exceeds Int range. makeYearMonthInterval throws unconditionally (not | ||
| -- ANSI-gated); this confirms the dispatched codegen path propagates Spark's exception. The | ||
| -- pattern is the lowercase word so it matches every version: Spark 4.x raises | ||
| -- INTERVAL_ARITHMETIC_OVERFLOW ("Integer overflow while operating with intervals") while Spark 3.x | ||
| -- raises a raw ArithmeticException ("integer overflow"). | ||
| query expect_error(overflow) | ||
| SELECT make_ym_interval(2147483647) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observation, no change requested.
fromArrowTypenow maps bothInterval(DayTime)(line 110) andDuration(Microsecond)(line 111) toDayTimeIntervalType, whiletoArrowTypeonly ever emitsDuration(Microsecond). So theInterval(DayTime)reverse case is dead for Comet-produced data and is presumably kept for externally-produced Arrow. That is fine and consistent with the precision rationale, just flagging the asymmetry in case it was unintentional.