You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/03/11 14:36:27 UTC
[GitHub] [spark] MaxGekk opened a new pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
MaxGekk opened a new pull request #31810:
URL: https://github.com/apache/spark/pull/31810
### 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:
<img width="836" alt="Screenshot 2021-03-11 at 17 29 04" src="https://user-images.githubusercontent.com/1580697/110802854-a54aa980-828f-11eb-956d-dd4fbf14aa72.png">
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"
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31810:
URL: https://github.com/apache/spark/pull/31810#issuecomment-796826639
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40558/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31810:
URL: https://github.com/apache/spark/pull/31810#issuecomment-796815485
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/135973/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31810:
URL: https://github.com/apache/spark/pull/31810#issuecomment-797233845
late LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31810:
URL: https://github.com/apache/spark/pull/31810#discussion_r592521765
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/types/YearMonthIntervalType.scala
##########
@@ -54,6 +54,8 @@ class YearMonthIntervalType private() extends AtomicType {
override def defaultSize: Int = 4
private[spark] override def asNullable: YearMonthIntervalType = this
+
+ override def typeName: String = "year-month interval"
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #31810:
URL: https://github.com/apache/spark/pull/31810#issuecomment-797033037
@cloud-fan @dongjoon-hyun @HyukjinKwon Could you review this PR, please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun closed pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #31810:
URL: https://github.com/apache/spark/pull/31810
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #31810: [SPARK-34718][SQL] Assign pretty names to YearMonthIntervalType and DayTimeIntervalType
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #31810:
URL: https://github.com/apache/spark/pull/31810#discussion_r592504404
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/types/YearMonthIntervalType.scala
##########
@@ -54,6 +54,8 @@ class YearMonthIntervalType private() extends AtomicType {
override def defaultSize: Int = 4
private[spark] override def asNullable: YearMonthIntervalType = this
+
+ override def typeName: String = "year-month interval"
Review comment:
according to the standard，year-month and day-time is italic，so I guess interval here is unnecessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org