Skip to content

[SPARK-38332][SQL] Add the DATEADD() and DATE_ADD() aliases for TIMESTAMPADD()#35661

Closed
MaxGekk wants to merge 6 commits intoapache:masterfrom
MaxGekk:dateadd
Closed

[SPARK-38332][SQL] Add the DATEADD() and DATE_ADD() aliases for TIMESTAMPADD()#35661
MaxGekk wants to merge 6 commits intoapache:masterfrom
MaxGekk:dateadd

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Feb 25, 2022

What changes were proposed in this pull request?

In the PR, I propose to add two aliases for the TIMESTAMPADD() function introduced by #35502:

  • DATEADD()
  • DATE_ADD()

Why are the changes needed?

  1. To make the migration process from other systems to Spark SQL easier.
  2. To achieve feature parity with other DBMSs.

Does this PR introduce any user-facing change?

No. The new aliases just extend Spark SQL API.

How was this patch tested?

  1. By running the existing test suites:
$ build/sbt "test:testOnly *SQLKeywordSuite"
  1. and new checks:
$ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z date.sql"

@MaxGekk MaxGekk changed the title [WIP][SPARK-38332][SQL] Add the DATEADD() and DATE_ADD() aliases for TIMESTAMPADD() [SPARK-38332][SQL] Add the DATEADD() and DATE_ADD() aliases for TIMESTAMPADD() Feb 27, 2022
@MaxGekk MaxGekk marked this pull request as ready for review February 27, 2022 15:40
@MaxGekk
Copy link
Member Author

MaxGekk commented Feb 27, 2022

@srielau @entong @superdupershant Any feedback is welcome.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

I'm fine w/ this. Look fine.

Copy link

@superdupershant superdupershant left a comment

Choose a reason for hiding this comment

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

Looks good except the return type shouldn't always be TIMESTAMP if the input expression and operation fit within a DATE type.
Sorry for missing this in the original TIMESTAMPADD change as well.



-- !query
select date_add(DAY, 367, date'2022-02-25')
Copy link

@superdupershant superdupershant Feb 28, 2022

Choose a reason for hiding this comment

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

I think the convention is if the third argument is of type DATE the expression's return type is:

DATE if the unit is DAY or larger
TIMESTAMP if the unit is smaller than DAY i.e. (HOUR, MINUTE, SECOND, etc...)

Copy link
Member Author

@MaxGekk MaxGekk Feb 28, 2022

Choose a reason for hiding this comment

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

I don't think we should do such kind of changes in the PR which just introduces aliases. Let's do that separately w/ tests if the feature is supported by other DBMSs as well.

Also I would prefer to not link such changes to DATEADD since they affect TIMESTAMPADD. It is slightly strange to me that a timestamp function will return date type. Let's discuss this separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean timestampadd can take date inputs but dateadd can't take timestamp inputs? Then it's not a simple alias any more.

Choose a reason for hiding this comment

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

@cloud-fan no both expressions can take date and timestamp inputs it is just a simple alias.

@MaxGekk yes I agree fixing it in a separate PR makes sense.

Copy link

@superdupershant superdupershant left a comment

Choose a reason for hiding this comment

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

LGTM let's just fix the return types in a separate PR.



-- !query
select date_add(DAY, 367, date'2022-02-25')

Choose a reason for hiding this comment

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

@cloud-fan no both expressions can take date and timestamp inputs it is just a simple alias.

@MaxGekk yes I agree fixing it in a separate PR makes sense.

@MaxGekk
Copy link
Member Author

MaxGekk commented Feb 28, 2022

Merging to master. Thank you, @HyukjinKwon @superdupershant @cloud-fan @gengliangwang for review.

@MaxGekk MaxGekk closed this in 6df10ce Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants