[SPARK-31474][SQL] Consistency between dayofweek/dow in extract expression and dayofweek function #28248

yaooqinn · 2020-04-17T18:28:05Z

What changes were proposed in this pull request?

spark-sql> SELECT extract(dayofweek from '2009-07-26');
1
spark-sql> SELECT extract(dow from '2009-07-26');
0
spark-sql> SELECT extract(isodow from '2009-07-26');
7
spark-sql> SELECT dayofweek('2009-07-26');
1
spark-sql> SELECT weekday('2009-07-26');
6

Currently, there are 4 types of day-of-week range:

the function dayofweek(2.3.0) and extracting dayofweek(2.4.0) result as of Sunday(1) to Saturday(7)
extracting dow(3.0.0) results as of Sunday(0) to Saturday(6)
extracting isodow (3.0.0) results as of Monday(1) to Sunday(7)
the function weekday(2.4.0) results as of Monday(0) to Sunday(6)

Actually, extracting dayofweek and dow are both derived from PostgreSQL but have different meanings.
https://issues.apache.org/jira/browse/SPARK-23903
https://issues.apache.org/jira/browse/SPARK-28623

In this PR, we make extracting dow as same as extracting dayofweek and the dayofweek function for historical reason and not breaking anything.

Also, add more documentation to the extracting function to make extract field more clear to understand.

Why are the changes needed?

Consistency insurance

Does this PR introduce any user-facing change?

yes, doc updated and extract dow is as same as dayofweek

How was this patch tested?

modified ut
local SQL doc verification

before

after

…ssion and dayofweek function

yaooqinn · 2020-04-17T18:29:36Z

cc @cloud-fan @dongjoon-hyun @HyukjinKwon thanks.

dongjoon-hyun · 2020-04-17T19:58:01Z

Hi, @yaooqinn . I also know the discussion history on the old PRs, but it would be great if you can add a link to describe ANSI SQL reference in the PR description (if possible)?

cc @gatorsmile

dongjoon-hyun · 2020-04-17T20:42:46Z

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

    case "DAY" | "D" | "DAYS" => DayOfMonth(source)
-    case "DAYOFWEEK" => DayOfWeek(source)
-    case "DOW" => Subtract(DayOfWeek(source), Literal(1))
+    case "DOW" | "DAYOFWEEK" => DayOfWeek(source)


It seems to be different from @cloud-fan 's request.
@cloud-fan . Could you confirm this?

yaooqinn · 2020-04-18T03:33:02Z

Hi, @dongjoon-hyun the ANSI SQL reference seems not to define the meaning of dow which is more likely to be defined by ISO. some of the other platforms like PostgreSQL/presto/DB2/SQL Server support this, others like Oracle don't. Those systems with dow support are also different from each other for the [0, 1]-based and [Monday, Sunday]-start method if no ’iso‘ suffix or prefix. But at least they do not vary inside their systems.

dongjoon-hyun · 2020-04-18T03:59:33Z

Thanks. Then, did you see the following?

[SPARK-28623][SQL] Support dow, isodow and doy by extract() #25367 (comment)

yaooqinn · 2020-04-18T05:40:26Z

Hi, @dongjoon-hyun, thanks for adding that reference. Actually, before this work, I have reached @cloud-fan and had a discussion about this issue offline. I have got his permit about the changes made in this PR and I hope other members of the spark community to see if this is a proper change.

SparkQA · 2020-04-18T07:05:02Z

Test build #121440 has finished for PR 28248 at commit 746eedf.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-04-18T07:16:26Z

Ya. Got it. Let's see the review comment~

dongjoon-hyun · 2020-04-18T07:16:32Z

Retest this please.

SparkQA · 2020-04-18T12:20:32Z

Test build #121448 has finished for PR 28248 at commit 746eedf.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

Hi, @yaooqinn .
The first target of expression description is DESC FUNCTION EXTENDED in SQL environment. The last commit shows the following. Please check the result and revert the HTML tags stuff.

spark-sql> DESC FUNCTION EXTENDED date_part;
Function: date_part
Class: org.apache.spark.sql.catalyst.expressions.DatePart
Usage: date_part(field, source) - Extracts a part of the date/timestamp or interval source.
Extended Usage:
    Arguments:
      * field - selects which part of the source should be extracted.
      <ul>
        <b> Supported string values of `field` for dates and timestamps are: </b>
          <li> "MILLENNIUM", ("MILLENNIA", "MIL", "MILS") - the conventional numbering of millennia </li>
          <li> "CENTURY", ("CENTURIES", "C", "CENT") - the conventional numbering of centuries </li>
          <li> "DECADE", ("DECADES", "DEC", "DECS") - the year field divided by 10 </li>
          <li> "YEAR", ("Y", "YEARS", "YR", "YRS") - the year field </li>

SparkQA · 2020-04-19T20:03:06Z

Test build #121478 has finished for PR 28248 at commit fdb5467.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class AuthRpcHandler extends AbstractAuthRpcHandler
public class SaslRpcHandler extends AbstractAuthRpcHandler
public abstract class AbstractAuthRpcHandler extends RpcHandler
case class Extract(field: Expression, source: Expression, child: Expression)

yaooqinn · 2020-04-20T02:55:51Z

@dongjoon-hyun thanks for checking the describe function command.

It seems that */- with proper intentions can let us have it in both ways - in SQL command and SQL doc.

Please check the result below to see whether you are satisfied with.

+-- !query
+DESC FUNCTION EXTENDED date_part
+-- !query schema
+struct<function_desc:string>
+-- !query output
+Class: org.apache.spark.sql.catalyst.expressions.DatePart
+Extended Usage:
+    Arguments:
+      * field - selects which part of the source should be extracted.
+          - Supported string values of `field` for dates and timestamps are:
+              - "MILLENNIUM", ("MILLENNIA", "MIL", "MILS") - the conventional numbering of millennia
+              - "CENTURY", ("CENTURIES", "C", "CENT") - the conventional numbering of centuries
+              - "DECADE", ("DECADES", "DEC", "DECS") - the year field divided by 1
+              - "YEAR", ("Y", "YEARS", "YR", "YRS") - the year field
+              - "ISOYEAR" - the ISO 8601 week-numbering year that the datetime falls in
+              - "QUARTER", ("QTR") - the quarter (1 - 4) of the year that the datetime falls in
+              - "MONTH", ("MON", "MONS", "MONTHS") - the month field
+              - "WEEK", ("W", "WEEKS") - the number of the ISO 8601 week-of-week-based-year. A week is considered to start on a Monday and week 1 is the first week with >3 days. In the ISO week-numbering system, it is possible for early-January dates to be part of the 52nd or 53rd week of the previous year, and for late-December dates to be part of the first week of the next year. For example, 2005-01-02 is part of the 53rd week of year 2004, while 2012-12-31 is part of the first week of 2013
+              - "DAY", ("D", "DAYS") - the day of the month field (1 - 31)
+              - "DAYOFWEEK",("DOW") - the day of the week for datetime as Sunday(1) to Saturday(7)
+              - "ISODOW" - ISO 8601 based day of the week for datetime as Monday(1) to Sunday(7)
+              - "DOY" - the day of the year (1 - 365/366)
+              - "HOUR", ("H", "HOURS", "HR", "HRS") - The hour field (0 - 23)
+              - "MINUTE", ("M", "MIN", "MINS", "MINUTES") - the minutes field (0 - 59)
+              - "SECOND", ("S", "SEC", "SECONDS", "SECS") - the seconds field, including fractional parts
+              - "MILLISECONDS", ("MSEC", "MSECS", "MILLISECON", "MSECONDS", "MS") - the seconds field, including fractional parts, multiplied by 1000. Note that this includes full seconds
+              - "MICROSECONDS", ("USEC", "USECS", "USECONDS", "MICROSECON", "US") - The seconds field, including fractional parts, multiplied by 1000000. Note that this includes full seconds
+              - "EPOCH" - the number of seconds with fractional part in microsecond precision since 1970-01-01 00:00:00 local time (can be negative)
+          - Supported string values of `field` for interval(which consists of `months`, `days`, `microseconds`) are:
+              - "YEAR", ("Y", "YEARS", "YR", "YRS") - the total `months` / 12
+              - "MONTH", ("MON", "MONS", "MONTHS") - the total `months` modulo 12
+              - "DAY", ("D", "DAYS") - the `days` part of interval
+              - "HOUR", ("H", "HOURS", "HR", "HRS") - how many hours the `microseconds` contains
+              - "MINUTE", ("M", "MIN", "MINS", "MINUTES") - how many minutes left after taking hours from `microseconds`
+              - "SECOND", ("S", "SEC", "SECONDS", "SECS") - how many second with fractions left after taking hours and minutes from `microseconds`
+      * source - a date/timestamp or interval column from where `field` should be extracted
+
+    Examples:
+      > SELECT date_part('YEAR', TIMESTAMP '2019-08-12 01:00:00.123456');
+       2019
+      > SELECT date_part('week', timestamp'2019-08-12 01:00:00.123456');
+       33
+      > SELECT date_part('doy', DATE'2019-08-12');
+       224
+      > SELECT date_part('SECONDS', timestamp'2019-10-01 00:00:01.000001');
+       1.000001
+      > SELECT date_part('days', interval 1 year 10 months 5 days);
+       5
+      > SELECT date_part('seconds', interval 5 hours 30 seconds 1 milliseconds 1 microseconds);
+       30.001001
+
+    Note:
+      The date_part function is equivalent to the SQL-standard function `extract`
+
+    Since: 3.0.0
+
+Function: date_part
+Usage: date_part(field, source) - Extracts a part of the date/timestamp or interval source.

cloud-fan · 2020-04-20T04:26:08Z

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

    case "DAY" | "D" | "DAYS" => DayOfMonth(source)
-    case "DAYOFWEEK" => DayOfWeek(source)
-    case "DOW" => Subtract(DayOfWeek(source), Literal(1))
+    case "DAYOFWEEK" | "DOW" => DayOfWeek(source)


I said that the DOW behavior looks more reasonable, but unfortunately, we already have DAYOFWEEK in Spark 2.4 and we can't change that. It's more important to keep internal consistency.

cloud-fan · 2020-04-20T04:28:49Z

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

+              - "YEAR", ("Y", "YEARS", "YR", "YRS") - the year field
+              - "ISOYEAR" - the ISO 8601 week-numbering year that the datetime falls in
+              - "QUARTER", ("QTR") - the quarter (1 - 4) of the year that the datetime falls in
+              - "MONTH", ("MON", "MONS", "MONTHS") - the month field


the month field (1 - 12)

cloud-fan · 2020-04-20T04:30:03Z

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

+              - "EPOCH" - the number of seconds with fractional part in microsecond precision since 1970-01-01 00:00:00 local time (can be negative)
+          - Supported string values of `field` for interval(which consists of `months`, `days`, `microseconds`) are:
+              - "YEAR", ("Y", "YEARS", "YR", "YRS") - the total `months` / 12
+              - "MONTH", ("MON", "MONS", "MONTHS") - the total `months` modulo 12


the total `months` % 12

to be consistent with

the total `months` / 12

cloud-fan · 2020-04-20T04:31:41Z

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

       30.001001
  """,
+  note = """
+    The _FUNC_ function is equivalent to the SQL-standard function `extract`


EXTRACT(field FROM source)

cloud-fan · 2020-04-20T04:31:59Z

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

       30.001001
  """,
+  note = """
+    The _FUNC_ function is equivalent to `date_part`.


date_part(field, source)

BTW is EXTRACT more widely used? If yes then we should put the document in Extract.

SparkQA · 2020-04-20T07:05:01Z

Test build #121494 has finished for PR 28248 at commit 0b90597.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-20T08:26:39Z

Test build #121507 has finished for PR 28248 at commit 2cedf2a.

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

yaooqinn · 2020-04-20T08:27:35Z

retest this please

SparkQA · 2020-04-20T14:06:45Z

Test build #121512 has finished for PR 28248 at commit 2cedf2a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-21T07:05:01Z

Test build #121565 has finished for PR 28248 at commit 10b733c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class CacheManager extends Logging with AdaptiveSparkPlanHelper
case class AdaptiveExecutionContext(session: SparkSession, qe: QueryExecution)

yaooqinn · 2020-04-21T07:07:02Z

retest this please

SparkQA · 2020-04-21T11:51:20Z

Test build #121573 has finished for PR 28248 at commit 10b733c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class CacheManager extends Logging with AdaptiveSparkPlanHelper
case class AdaptiveExecutionContext(session: SparkSession, qe: QueryExecution)

cloud-fan · 2020-04-21T11:55:48Z

thanks, merging to master/3.0!

…ession and dayofweek function ### What changes were proposed in this pull request? ```sql spark-sql> SELECT extract(dayofweek from '2009-07-26'); 1 spark-sql> SELECT extract(dow from '2009-07-26'); 0 spark-sql> SELECT extract(isodow from '2009-07-26'); 7 spark-sql> SELECT dayofweek('2009-07-26'); 1 spark-sql> SELECT weekday('2009-07-26'); 6 ``` Currently, there are 4 types of day-of-week range: 1. the function `dayofweek`(2.3.0) and extracting `dayofweek`(2.4.0) result as of Sunday(1) to Saturday(7) 2. extracting `dow`(3.0.0) results as of Sunday(0) to Saturday(6) 3. extracting` isodow` (3.0.0) results as of Monday(1) to Sunday(7) 4. the function `weekday`(2.4.0) results as of Monday(0) to Sunday(6) Actually, extracting `dayofweek` and `dow` are both derived from PostgreSQL but have different meanings. https://issues.apache.org/jira/browse/SPARK-23903 https://issues.apache.org/jira/browse/SPARK-28623 In this PR, we make extracting `dow` as same as extracting `dayofweek` and the `dayofweek` function for historical reason and not breaking anything. Also, add more documentation to the extracting function to make extract field more clear to understand. ### Why are the changes needed? Consistency insurance ### Does this PR introduce any user-facing change? yes, doc updated and extract `dow` is as same as `dayofweek` ### How was this patch tested? 1. modified ut 2. local SQL doc verification #### before ![image](https://user-images.githubusercontent.com/8326978/79601949-3535b100-811c-11ea-957b-a33d68641181.png) #### after ![image](https://user-images.githubusercontent.com/8326978/79601847-12a39800-811c-11ea-8ff6-aa329255d099.png) Closes #28248 from yaooqinn/SPARK-31474. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 1985437) Signed-off-by: Wenchen Fan <[email protected]>

…name in the note field of expression info ### What changes were proposed in this pull request? \_FUNC\_ is used in note() of `ExpressionDescription` since #28248, it can be more cases later, we should replace it with function name for documentation ### Why are the changes needed? doc fix ### Does this PR introduce any user-facing change? no ### How was this patch tested? pass Jenkins, and verify locally with Jekyll serve Closes #28305 from yaooqinn/SPARK-31474-F. Authored-by: Kent Yao <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

…name in the note field of expression info ### What changes were proposed in this pull request? \_FUNC\_ is used in note() of `ExpressionDescription` since #28248, it can be more cases later, we should replace it with function name for documentation ### Why are the changes needed? doc fix ### Does this PR introduce any user-facing change? no ### How was this patch tested? pass Jenkins, and verify locally with Jekyll serve Closes #28305 from yaooqinn/SPARK-31474-F. Authored-by: Kent Yao <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit 3b57921) Signed-off-by: HyukjinKwon <[email protected]>

[SPARK-31474][SQL] Consistancy between dayofweek/dow in extract expre…

f7bc993

…ssion and dayofweek function

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

dongjoon-hyun reviewed Apr 17, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

regen test result

746eedf

Merge branch 'master' into SPARK-31474

fdb5467

dongjoon-hyun requested changes Apr 19, 2020

View reviewed changes

address comments

0b90597

cloud-fan reviewed Apr 20, 2020

View reviewed changes

address comments

2cedf2a

Merge branch 'master' into SPARK-31474

10b733c

cloud-fan approved these changes Apr 21, 2020

View reviewed changes

cloud-fan closed this in 1985437 Apr 21, 2020

cloud-fan changed the title ~~[SPARK-31474][SQL] Consistency between dayofweek/dow in extract exprsession and dayofweek function~~ [SPARK-31474][SQL] Consistency between dayofweek/dow in extract expression and dayofweek function Apr 21, 2020

yaooqinn mentioned this pull request Apr 23, 2020

[SPARK-31474][SQL][FOLLOWUP] Replace _FUNC_ placeholder with functionname in the note field of expression info #28305

Closed

yaooqinn mentioned this pull request Apr 23, 2020

[SPARK-31474][SQL][FOLLOWUP] Replace _FUNC_ placeholder with functionname in the note field of expression info #28314

Closed

Uh oh!

[SPARK-31474][SQL] Consistency between dayofweek/dow in extract expression and dayofweek function #28248

[SPARK-31474][SQL] Consistency between dayofweek/dow in extract expression and dayofweek function #28248

Uh oh!

Conversation

yaooqinn commented Apr 17, 2020

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?

before

after

Uh oh!

yaooqinn commented Apr 17, 2020

Uh oh!

dongjoon-hyun commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

yaooqinn commented Apr 18, 2020

Uh oh!

dongjoon-hyun commented Apr 18, 2020

Uh oh!

yaooqinn commented Apr 18, 2020

Uh oh!

SparkQA commented Apr 18, 2020

Uh oh!

dongjoon-hyun commented Apr 18, 2020

Uh oh!

dongjoon-hyun commented Apr 18, 2020

Uh oh!

SparkQA commented Apr 18, 2020

Uh oh!

dongjoon-hyun left a comment • 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!

SparkQA commented Apr 19, 2020

Uh oh!

yaooqinn commented Apr 20, 2020

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!

SparkQA commented Apr 20, 2020

Uh oh!

SparkQA commented Apr 20, 2020

Uh oh!

yaooqinn commented Apr 20, 2020

Uh oh!

SparkQA commented Apr 20, 2020

Uh oh!

SparkQA commented Apr 21, 2020

Uh oh!

yaooqinn commented Apr 21, 2020

Uh oh!

SparkQA commented Apr 21, 2020

Uh oh!

cloud-fan commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dongjoon-hyun commented Apr 17, 2020 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading