Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Apr 5, 2021

What changes were proposed in this pull request?

  1. Added new method toYearMonthIntervalString() to IntervalUtils which converts an year-month interval as a number of month to a string in the form "INTERVAL '[sign]yearField-monthField' YEAR TO MONTH".
  2. Extended the Cast expression to support casting of YearMonthIntervalType to StringType.

Why are the changes needed?

To conform the ANSI SQL standard which requires to support such casting.

Does this PR introduce any user-facing change?

Should not because new year-month interval has not been released yet.

How was this patch tested?

Added new tests for casting:

$ build/sbt "testOnly *CastSuite*"

@github-actions github-actions bot added the SQL label Apr 5, 2021
@SparkQA
Copy link

SparkQA commented Apr 5, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41498/

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

Test build #136921 has finished for PR 32056 at commit d203e1e.

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

@MaxGekk
Copy link
Member Author

MaxGekk commented Apr 6, 2021

@gengliangwang @cloud-fan @yaooqinn @AngersZhuuuu Could you review this PR, please.

sign = "-"
absMonths = -absMonths
}
s"interval '$sign${absMonths / MONTHS_PER_YEAR}-${absMonths % MONTHS_PER_YEAR}' year to month"
Copy link
Member

Choose a reason for hiding this comment

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

The last part "year to month" looks a bit confusing to me. What does other DBMS systems do?

Copy link
Member Author

@MaxGekk MaxGekk Apr 6, 2021

Choose a reason for hiding this comment

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

The last part "year to month" looks a bit confusing to me

This part conforms to the SQL standard, see:

<interval literal> ::=
  INTERVAL [ <sign> ] <interval string> <interval qualifier>

<interval qualifier> ::=
    <start field> TO <end field>
  | <single datetime field>
<start field> ::=
<non-second primary datetime field>
      [ <left paren> <interval leading field precision> <right paren> ]
<end field> ::=
<non-second primary datetime field>
  | SECOND [ <left paren> <interval fractional seconds precision> <right paren> ]
<single datetime field> ::= <non-second primary datetime field>
        [ <left paren> <interval leading field precision> <right paren> ]
  | SECOND [ <left paren> <interval leading field precision>
      [ <comma> <interval fractional seconds precision> ] <right paren> ]
<primary datetime field> ::= <non-second primary datetime field>
| SECOND
<non-second primary datetime field> ::= YEAR
  | MONTH
  | DAY
  | HOUR
  | MINUTE

<interval string> ::=
  <quote> <unquoted interval string> <quote>
<unquoted interval string> ::=
[ <sign> ] { <year-month literal> | <day-time literal> }

What does other DBMS systems do?

For instance, see Oracle https://docs.oracle.com/en/database/oracle/oracle-database/12.2/sqlrf/Literals.html#GUID-4C258D8F-3DF2-4D45-BE3E-14864DD77100

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the explanation!

Copy link
Member

Choose a reason for hiding this comment

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

As per the standard, it seems the output of "-13 months" should be
interval - '1-1' year to month
instead of
interval '-1-1' year to month

Copy link
Member Author

Choose a reason for hiding this comment

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

Both are allowed by the standard. See the grammar rules:

<unquoted interval string> ::=
[ <sign> ] { <year-month literal> | <day-time literal> }
<interval literal> ::=
  INTERVAL [ <sign> ] <interval string> <interval qualifier>

Copy link
Member

Choose a reason for hiding this comment

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

I see. Shall we add one space in between the sign and interval string as per the standard?

Copy link
Member Author

@MaxGekk MaxGekk Apr 6, 2021

Choose a reason for hiding this comment

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

I think the space doesn't matter. See for example:
Screenshot 2021-04-06 at 12 00 12

The space in the rules is just for readability, I think.

@gengliangwang
Copy link
Member

Looks good to me except two comments.

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41515/

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41515/

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41516/

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41516/

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

Test build #136938 has finished for PR 32056 at commit df2992c.

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

@MaxGekk
Copy link
Member Author

MaxGekk commented Apr 6, 2021

Merging to master. Thank you @gengliangwang @cloud-fan and @yaooqinn for your reviews.

@MaxGekk MaxGekk closed this in 4b5fc1d Apr 6, 2021
@SparkQA
Copy link

SparkQA commented Apr 6, 2021

Test build #136939 has finished for PR 32056 at commit d918473.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants