You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "uros-db (via GitHub)" <gi...@apache.org> on 2025/07/11 15:40:27 UTC
[PR] Initial commit [spark]
uros-db opened a new pull request, #51455:
URL: https://github.com/apache/spark/pull/51455
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Introduce support for casting TIME to DECIMAL.
### Why are the changes needed?
Expand TIME time support and interoperability with the existing type system.
### Does this PR introduce _any_ user-facing change?
Yes, casting is now enabled from TIME to DECIMAL.
### How was this patch tested?
Added catalyst expression-level cast tests in both ANSI and non-ANSI modes, as well as e2e SQL tests.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #51455:
URL: https://github.com/apache/spark/pull/51455#issuecomment-3084437785
+1, LGTM. Merging to master.
Thank you, @uros-db and @mihailom-db for review.
--
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
Re: [PR] [WIP][SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51455:
URL: https://github.com/apache/spark/pull/51455#discussion_r2210317616
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1011,6 +1015,8 @@ case class Cast(
case TimestampType =>
// Note that we lose precision here.
buildCast[Long](_, t => changePrecision(Decimal(timestampToDouble(t)), target))
+ case _: TimeType =>
Review Comment:
Of course, I will add appropriate tests in case that the target DecimalType has insufficient `precision` - for ANSI-off we should get NULLs, and for ANSI-on we should get exceptions.
--
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
Re: [PR] [WIP][SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51455:
URL: https://github.com/apache/spark/pull/51455#discussion_r2210314523
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1011,6 +1015,8 @@ case class Cast(
case TimestampType =>
// Note that we lose precision here.
buildCast[Long](_, t => changePrecision(Decimal(timestampToDouble(t)), target))
+ case _: TimeType =>
Review Comment:
Precision loss with decimals is inevitable if we want to deal with nanoseconds, in case that the target DecimalType has insufficient `scale`. This goes for both TimestampType and TimeType.
--
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
Re: [PR] [SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #51455:
URL: https://github.com/apache/spark/pull/51455#issuecomment-3067048988
@fartzy Could you review this PR since you are working on similar one: https://github.com/apache/spark/pull/51461
--
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
Re: [PR] [SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #51455:
URL: https://github.com/apache/spark/pull/51455#discussion_r2213132634
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1046,6 +1053,8 @@ case class Cast(
case TimestampType =>
// Note that we lose precision here.
buildCast[Long](_, t => changePrecision(Decimal(timestampToDouble(t)), target))
+ case _: TimeType =>
+ buildCast[Long](_, t => changePrecision(Decimal(timeToDouble(t)), target))
Review Comment:
Would it be possible to avoid the conversion to `Double`, and create `Decimal` from from `Long` directly? It seems there is such method:
```scala
def apply(unscaled: Long, precision: Int, scale: Int): Decimal =
new Decimal().set(unscaled, precision, scale)
```
--
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
Re: [PR] [WIP][SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51455:
URL: https://github.com/apache/spark/pull/51455#discussion_r2210314523
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1011,6 +1015,8 @@ case class Cast(
case TimestampType =>
// Note that we lose precision here.
buildCast[Long](_, t => changePrecision(Decimal(timestampToDouble(t)), target))
+ case _: TimeType =>
Review Comment:
Precision loss with decimals is inevitable if we want to deal with nanoseconds. Goes for both TimestampType and TimeType.
--
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
Re: [PR] [SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51455:
URL: https://github.com/apache/spark/pull/51455#discussion_r2213523633
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1046,6 +1053,8 @@ case class Cast(
case TimestampType =>
// Note that we lose precision here.
buildCast[Long](_, t => changePrecision(Decimal(timestampToDouble(t)), target))
+ case _: TimeType =>
+ buildCast[Long](_, t => changePrecision(Decimal(timeToDouble(t)), target))
Review Comment:
This is a good idea, but I don't think this will actually work. I tried doing something like:
```
new Decimal().set(t / NANOS_PER_SECOND, target.precision, target.scale)
```
However, it turns out that we need to use `changePrecision` (as implemented in this PR currently), because it contains vital logic to handle rounding and overflow.
Also, I think that this explains why `changePrecision` is used everywhere in `Cast.scala`, while the `new Decimal().set` approach isn't used anywhere in casts currently.
--
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
Re: [PR] [SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #51455: [SPARK-52620][SQL] Support casting of TIME to DECIMAL
URL: https://github.com/apache/spark/pull/51455
--
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
Re: [PR] [WIP][SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #51455:
URL: https://github.com/apache/spark/pull/51455#discussion_r2210317616
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1011,6 +1015,8 @@ case class Cast(
case TimestampType =>
// Note that we lose precision here.
buildCast[Long](_, t => changePrecision(Decimal(timestampToDouble(t)), target))
+ case _: TimeType =>
Review Comment:
Of course, I will add appropriate tests - for ANSI-off we should get NULLs, and for ANSI-on we should get exceptions.
--
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
Re: [PR] [SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #51455:
URL: https://github.com/apache/spark/pull/51455#issuecomment-3081159660
@MaxGekk @fartzy Please review the TIME-DECIMAL cast PR.
--
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
Re: [PR] [WIP][SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on code in PR #51455:
URL: https://github.com/apache/spark/pull/51455#discussion_r2210003617
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1011,6 +1015,8 @@ case class Cast(
case TimestampType =>
// Note that we lose precision here.
buildCast[Long](_, t => changePrecision(Decimal(timestampToDouble(t)), target))
+ case _: TimeType =>
Review Comment:
Should we make sure we do not lose precision here? I see TimestampType is losing precision, does that apply to TimeType as well?
--
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
Re: [PR] [WIP][SPARK-52620][SQL] Support casting of TIME to DECIMAL [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #51455:
URL: https://github.com/apache/spark/pull/51455#issuecomment-3067131438
Please note that the PR is still a work in progress, I added the [WIP] label to the PR title now.
Nevertheless, all comments and suggestions are always welcome!
--
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