Skip to content

[SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType - #31810

Closed
MaxGekk wants to merge 2 commits into
apache:masterfrom
MaxGekk:interval-types-name
Closed

[SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType#31810
MaxGekk wants to merge 2 commits into
apache:masterfrom
MaxGekk:interval-types-name

Conversation

@MaxGekk

@MaxGekk MaxGekk commented Mar 11, 2021

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

In the PR, I propose to override the typeName() method in YearMonthIntervalType and DayTimeIntervalType, and assign them names according to the ANSI SQL standard:
Screenshot 2021-03-11 at 17 29 04
but keep the type name as singular according existing naming convention for other types.

Why are the changes needed?

To improve Spark SQL user experience, and have readable types in error messages.

Does this PR introduce any user-facing change?

Should not since the types has not been released yet.

How was this patch tested?

By running the modified tests:

$ build/sbt "test:testOnly *ExpressionTypeCheckingSuite"
$ build/sbt "sql/testOnly *SQLQueryTestSuite -- -z windowFrameCoercion.sql"
$ build/sbt "sql/testOnly *SQLQueryTestSuite -- -z literals.sql"

@AmplabJenkins

Copy link
Copy Markdown

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135973/

@AmplabJenkins

Copy link
Copy Markdown

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40558/


private[spark] override def asNullable: YearMonthIntervalType = this

override def typeName: String = "year-month interval"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

according to the standard,year-month and day-time is italic,so I guess interval here is unnecessary.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  1. I haven't found any places in the SQL standard where year-month and day-time are used without the interval(s) word(s).
  2. Actual type name is INTERVAL, year-month and day-time just define (sub-)classes of the type.
  3. Since the typeName() method is used in error messages, I do believe we should leave interval in type names for readability.

@github-actions github-actions Bot added the SQL label Mar 11, 2021
@MaxGekk

MaxGekk commented Mar 11, 2021

Copy link
Copy Markdown
Member Author

@cloud-fan @dongjoon-hyun @HyukjinKwon Could you review this PR, please.

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM. Thank you.
Merged to master.

@HyukjinKwon HyukjinKwon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM2

@cloud-fan

Copy link
Copy Markdown
Contributor

late LGTM

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.

6 participants