[SPARK-31507][SQL] Remove uncommon fields support and update some fields with meaningful names for extract function #28284

yaooqinn · 2020-04-21T10:36:31Z

What changes were proposed in this pull request?

Extracting millennium, century, decade, millisecond, microsecond and epoch from datetime is neither ANSI standard nor quite common in modern SQL platforms. Most of the systems listing below does not support these except PostgreSQL and redshift.

https://cwiki.apache.org/confluence/display/Hive/LanguageManual+UDF

https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions050.htm

https://prestodb.io/docs/current/functions/datetime.html

https://docs.cloudera.com/documentation/enterprise/5-8-x/topics/impala_datetime_functions.html

https://docs.snowflake.com/en/sql-reference/functions-date-time.html#label-supported-date-time-parts

https://www.postgresql.org/docs/9.1/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT

This PR removes these extract fields support from extract function for date and timestamp values

isoyear is PostgreSQL specific but yearofweek is more commonly used across platforms
isodow is PostgreSQL specific but iso as a suffix is more commonly used across platforms so, dow_iso and dayofweek_iso is used to replace it.

For historical reasons, we have [dayofweek, dow] implemented for representing a non-ISO day-of-week and a newly added isodow from PostgreSQL for ISO day-of-week. Many other systems only have one week-numbering system support and use either full names or abbreviations. Things in spark become a little bit complicated.

because of the existence of isodow, so we need to add iso-prefix to dayofweek to make a pair for it too. [dayofweek, isodayofweek, dow and isodow]
because there are rare iso-prefixed systems and more systems choose iso-suffixed way, so we may result in [dayofweek, dayofweekiso, dow, dowiso]
dayofweekiso looks nice and has use cases in the platforms listed above, e.g. snowflake, but dowiso looks weird and no use cases found.
with a discussion the community，we have agreed with an underscore before iso may look much better because isodow is new and there is no standard for iso kind of things, so this may be good for us to make it simple and clear for end-users if they are well documented too.

Thus, we finally result in [dayofweek, dow] for Non-ISO day-of-week system and [dayofweek_iso, dow_iso] for ISO system

Why are the changes needed?

Remove some nonstandard and uncommon features as we can add them back if necessary

Does this PR introduce any user-facing change?

NO, we should target this to 3.0.0 and these are added during 3.0.0

How was this patch tested?

Remove unused tests

…icrosecond and epoch fields support from extract fucntion

SparkQA · 2020-04-21T12:07:40Z

Test build #121581 has finished for PR 28284 at commit 3146b65.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan

LGTM. We can add them back later if we find them useful. It's very difficult to remove them if they are released with 3.0

cloud-fan · 2020-04-21T13:48:52Z

cc @MaxGekk @dongjoon-hyun @HyukjinKwon

SparkQA · 2020-04-21T17:54:07Z

Test build #121586 has finished for PR 28284 at commit 521bc2c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class YearOfWeek(child: Expression) extends UnaryExpression with ImplicitCastInputTypes

dongjoon-hyun · 2020-04-21T18:05:16Z

cc @maropu

dongjoon-hyun · 2020-04-21T18:09:04Z

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

    case "DAY" | "D" | "DAYS" => DayOfMonth(source)
    case "DAYOFWEEK" | "DOW" => DayOfWeek(source)
-    case "ISODOW" => Add(WeekDay(source), Literal(1))
+    case "DAYOFWEEK_ISO" | "DOW_ISO" => Add(WeekDay(source), Literal(1))


Unlike the PR title, this PR is adding DAYOFWEEK_ISO newly. Which platform are you referring for DAYOFWEEK_ISO? IIRC, @gatorsmile commented that we should not consider IBM DB i.

isodow is PostgreSQL specific but iso as a suffix is more commonly used across platforms

Hi @dongjoon-hyun, for historical reasons, we have [dayofweek, dow] implemented for representing a non-ISO day-of-week and a newly added isodow from PostgreSQL for ISO day-of-week. Many other systems only have one week-numbering system support and use either full names or abbreviations. Things in spark become a little bit complicated.

because of the existence of isodow, so we need to add iso-prefix to dayofweek to make a pair for it too. [dayofweek, isodayofweek, dow and isodow]

because there are rare iso-prefixed systems and more systems choose iso-suffixed way, so we may result in [dayofweek, dayofweekiso, dow, dowiso]

dayofweekiso looks nice and has use cases in the platforms listed above, e.g. snowflake, but dowiso looks weird and no use cases found.

after a discussion with @cloud-fan, we have both agreed with an underscore before iso may look much better because isodow is new and there is no standard for iso kind of things, so this may be good for us to make it simple and clear for end-users if they are well documented too.

Thus, we finally result in [dayofweek, dow] for Non-ISO week-numbering system and [dayofweek_iso, dow_iso] for ISO system

@yaooqinn . Ya. It's clear for that part. Could you enumerate the name of systems which supports DAYOFWEEK_ISO? (except IBM DB i). I'm wondering that specifically.

It's because you wrote like Many other systems in the PR description.

Specifically, snowflake use dayofweek_iso except db2. I couldn't find more because most platforms do not have two day-of-week system implemented.

Got it. Thanks. I understand that Snowflake is considered important. Could you add some of the above comment(#28284 (comment)) to the PR description then?

thanks @dongjoon-hyun. PR description updated

cloud-fan · 2020-04-22T10:24:42Z

thanks, merging to master/3.0!

…lds with meaningful names for extract function ### What changes were proposed in this pull request? Extracting millennium, century, decade, millisecond, microsecond and epoch from datetime is neither ANSI standard nor quite common in modern SQL platforms. Most of the systems listing below does not support these except PostgreSQL and redshift. https://cwiki.apache.org/confluence/display/Hive/LanguageManual+UDF https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions050.htm https://prestodb.io/docs/current/functions/datetime.html https://docs.cloudera.com/documentation/enterprise/5-8-x/topics/impala_datetime_functions.html https://docs.snowflake.com/en/sql-reference/functions-date-time.html#label-supported-date-time-parts https://www.postgresql.org/docs/9.1/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT This PR removes these extract fields support from extract function for date and timestamp values `isoyear` is PostgreSQL specific but `yearofweek` is more commonly used across platforms `isodow` is PostgreSQL specific but `iso` as a suffix is more commonly used across platforms so, `dow_iso` and `dayofweek_iso` is used to replace it. For historical reasons, we have [`dayofweek`, `dow`] implemented for representing a non-ISO day-of-week and a newly added `isodow` from PostgreSQL for ISO day-of-week. Many other systems only have one week-numbering system support and use either full names or abbreviations. Things in spark become a little bit complicated. 1. because of the existence of `isodow`, so we need to add iso-prefix to `dayofweek` to make a pair for it too. [`dayofweek`, `isodayofweek`, `dow` and `isodow`] 2. because there are rare `iso`-prefixed systems and more systems choose `iso`-suffixed way, so we may result in [`dayofweek`, `dayofweekiso`, `dow`, `dowiso`] 3. `dayofweekiso` looks nice and has use cases in the platforms listed above, e.g. snowflake, but `dowiso` looks weird and no use cases found. 4. with a discussion the community，we have agreed with an underscore before `iso` may look much better because `isodow` is new and there is no standard for `iso` kind of things, so this may be good for us to make it simple and clear for end-users if they are well documented too. Thus, we finally result in [`dayofweek`, `dow`] for Non-ISO day-of-week system and [`dayofweek_iso`, `dow_iso`] for ISO system ### Why are the changes needed? Remove some nonstandard and uncommon features as we can add them back if necessary ### Does this PR introduce any user-facing change? NO, we should target this to 3.0.0 and these are added during 3.0.0 ### How was this patch tested? Remove unused tests Closes #28284 from yaooqinn/SPARK-31507. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 37d2e03) Signed-off-by: Wenchen Fan <[email protected]>

dongjoon-hyun · 2020-04-22T16:13:17Z

Thank you all!

[SPARK-31507][SQL] Remove millennium, century, decade, millisecond, m…

3146b65

…icrosecond and epoch fields support from extract fucntion

probot-autolabeler bot added the SQL label Apr 21, 2020

yaooqinn marked this pull request as draft April 21, 2020 10:36

yaooqinn added 2 commits April 21, 2020 20:18

Merge branch 'master' into SPARK-31507

e215a9c

update field

521bc2c

yaooqinn marked this pull request as ready for review April 21, 2020 13:28

yaooqinn changed the title ~~[SPARK-31507][SQL] Remove millennium, century, decade, millisecond, m…~~ [SPARK-31507][SQL] Remove millennium, century, decade, millisecond, microsecond and epoch fields support from extract function Apr 21, 2020

yaooqinn changed the title ~~[SPARK-31507][SQL] Remove millennium, century, decade, millisecond, microsecond and epoch fields support from extract function~~ [SPARK-31507][SQL] Remove uncommon fields support and update some fields with meaningful names for extract function Apr 21, 2020

cloud-fan approved these changes Apr 21, 2020

View reviewed changes

dongjoon-hyun reviewed Apr 21, 2020

View reviewed changes

dongjoon-hyun mentioned this pull request Apr 21, 2020

[SPARK-31476][SQL][FOLLOWUP] Add tests for extract('field', source) #28276

Closed

cloud-fan closed this in 37d2e03 Apr 22, 2020

yaooqinn deleted the SPARK-31507 branch April 22, 2020 16:39

[SPARK-31507][SQL] Remove uncommon fields support and update some fields with meaningful names for extract function #28284

[SPARK-31507][SQL] Remove uncommon fields support and update some fields with meaningful names for extract function #28284

Uh oh!

Conversation

yaooqinn commented Apr 21, 2020 • 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!

SparkQA commented Apr 21, 2020

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 21, 2020

Uh oh!

SparkQA commented Apr 21, 2020

Uh oh!

dongjoon-hyun commented Apr 21, 2020

Uh oh!

dongjoon-hyun Apr 21, 2020 • 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!

yaooqinn Apr 22, 2020 • 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!

dongjoon-hyun Apr 22, 2020 • 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!

yaooqinn Apr 22, 2020 • 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!

dongjoon-hyun Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

yaooqinn Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Apr 22, 2020

Uh oh!

dongjoon-hyun commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yaooqinn commented Apr 21, 2020 •

edited

Loading

dongjoon-hyun Apr 21, 2020 •

edited

Loading

yaooqinn Apr 22, 2020 •

edited

Loading

dongjoon-hyun Apr 22, 2020 •

edited

Loading

yaooqinn Apr 22, 2020 •

edited

Loading