[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r450119402
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2612,6 +2612,11 @@ object Sequence {
val stepDays = step.days
val stepMicros = step.microseconds
+ if (scale == MICROS_PER_DAY && stepMonths == 0 && stepDays == 0) {
+throw new IllegalArgumentException(
+ "sequence step must be a day interval if start and end values are
dates")
+ }
+
if (stepMonths == 0 && stepMicros == 0 && scale == MICROS_PER_DAY) {
Review comment:
Done.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r450118579
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2618,9 +2618,11 @@ object Sequence {
}
if (stepMonths == 0 && stepMicros == 0 && scale == MICROS_PER_DAY) {
+// Adding pure days to date start/end
backedSequenceImpl.eval(start, stop, fromLong(stepDays))
} else if (stepMonths == 0 && stepDays == 0 && scale == 1) {
+// Adding pure microseconds to timestamp start/end
Review comment:
@cloud-fan Thanks, I add more comments for pure days and months branch,
the former exception check seems the exception content has enough informs, and
the last branch seems have detail content.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r450118579
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2618,9 +2618,11 @@ object Sequence {
}
if (stepMonths == 0 && stepMicros == 0 && scale == MICROS_PER_DAY) {
+// Adding pure days to date start/end
backedSequenceImpl.eval(start, stop, fromLong(stepDays))
} else if (stepMonths == 0 && stepDays == 0 && scale == 1) {
+// Adding pure microseconds to timestamp start/end
Review comment:
@cloud-fan Thanks, I add more comments for pure days and months branch,
the former exception check seems the exception content has enough informs, and
the last branch seems has detail content.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r450010508
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2612,6 +2612,11 @@ object Sequence {
val stepDays = step.days
val stepMicros = step.microseconds
+ if (scale == MICROS_PER_DAY && stepMonths == 0 && stepDays == 0) {
+throw new IllegalArgumentException(
Review comment:
I am both ok, thank you for your attention. :-)
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r449940558
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2612,6 +2614,9 @@ object Sequence {
val stepDays = step.days
val stepMicros = step.microseconds
+ require(scale != MICROS_PER_DAY || stepMonths != 0 || stepDays != 0,
+"sequence step must be a day interval if start and end values are
dates")
+
if (stepMonths == 0 && stepMicros == 0 && scale == MICROS_PER_DAY) {
Review comment:
@dongjoon-hyun Sorry, the correct PR is
[SPARK-31982](https://github.com/apache/spark/pull/28856), this PR is forbid
step and the other is cross the timezone.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r449940558
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2612,6 +2614,9 @@ object Sequence {
val stepDays = step.days
val stepMicros = step.microseconds
+ require(scale != MICROS_PER_DAY || stepMonths != 0 || stepDays != 0,
+"sequence step must be a day interval if start and end values are
dates")
+
if (stepMonths == 0 && stepMicros == 0 && scale == MICROS_PER_DAY) {
Review comment:
@dongjoon-hyun Sorry, the correct PR is
[https://github.com/apache/spark/pull/28856](), this PR is forbid step and the
other is cross the timezone.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r448035198
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala
##
@@ -1854,4 +1854,18 @@ class CollectionExpressionsSuite extends SparkFunSuite
with ExpressionEvalHelper
Literal(stringToInterval("interval 1 year"))),
Seq(Date.valueOf("2018-01-01")))
}
+
+ test("SPARK-32133: Sequence step must be a day interval " +
Review comment:
Thanks, I have moved the negative test to `Sequence of dates` and `
point to the JIRA number in the code comment.`
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r447536856
##
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala
##
@@ -1854,4 +1854,18 @@ class CollectionExpressionsSuite extends SparkFunSuite
with ExpressionEvalHelper
Literal(stringToInterval("interval 1 year"))),
Seq(Date.valueOf("2018-01-01")))
}
+
+ test("SPARK-32133: Sequence step must be a day interval " +
+"if start and end values are dates") {
+checkExceptionInExpression[IllegalArgumentException](Sequence(
+ Cast(Literal("2011-03-01"), DateType),
+ Cast(Literal("2011-04-01"), DateType),
+ Option(Literal(stringToInterval("interval 1 hour", null,
+ "sequence step must be a day interval if start and end values are dates")
+checkEvaluation(Sequence(
+ Cast(Literal("2011-03-01"), DateType),
+ Cast(Literal("2011-03-02"), DateType),
+ Option(Literal(stringToInterval("interval 1 day",
+ Seq(Date.valueOf("2011-03-01"), Date.valueOf("2011-03-02")))
Review comment:
@cloud-fan Thank you for your suggestion, I have done. Besides, I add a
positive test case, hope this could be better.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r447317750
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2623,8 +2628,16 @@ object Sequence {
// about a month length in days and a day length in microseconds
val intervalStepInMicros =
stepMicros + stepMonths * microsPerMonth + stepDays * microsPerDay
-val startMicros: Long = num.toLong(start) * scale
-val stopMicros: Long = num.toLong(stop) * scale
+
+// Date to timestamp is not equal from GMT and Chicago timezones
Review comment:
@cloud-fan Thanks, I have followed the suggestion and make a new jira
ticket for this.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] TJX2014 commented on a change in pull request #28926: [SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence
TJX2014 commented on a change in pull request #28926:
URL: https://github.com/apache/spark/pull/28926#discussion_r447316906
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
##
@@ -2612,6 +2614,9 @@ object Sequence {
val stepDays = step.days
val stepMicros = step.microseconds
+ require(scale != MICROS_PER_DAY || stepMonths != 0 || stepDays != 0,
+"sequence step must be a day interval if start and end values are
dates")
+
if (stepMonths == 0 && stepMicros == 0 && scale == MICROS_PER_DAY) {
Review comment:
Seems we need the `require` check in eval, and I remove `SPARK-32198`
branch code from this PR, is it ok ?
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
