Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -2612,10 +2612,17 @@ object Sequence {
val stepDays = step.days
val stepMicros = step.microseconds

if (scale == MICROS_PER_DAY && stepMonths == 0 && stepDays == 0) {
throw new IllegalArgumentException(
Copy link
Member

Choose a reason for hiding this comment

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

@TJX2014 can we just use require instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked to throw an exception explicitly, to match the codegen code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am both ok, thank you for your attention. :-)

"sequence step must be a day interval if start and end values are dates")
}

if (stepMonths == 0 && stepMicros == 0 && scale == MICROS_PER_DAY) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this branch now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems we need the require check in eval, and I remove SPARK-32198 branch code from this PR, is it ok ?

Copy link
Member

Choose a reason for hiding this comment

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

@TJX2014 . What is SPARK-32198? It's not created yet.

I remove SPARK-32198 branch code from this PR

Copy link
Member

Choose a reason for hiding this comment

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

Also, I have the same question like @cloud-fan .

Copy link
Contributor Author

@TJX2014 TJX2014 Jul 6, 2020

Choose a reason for hiding this comment

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

@dongjoon-hyun Sorry, the correct PR is SPARK-31982, this PR is forbid step and the other is cross the timezone.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably add comments for each branch. For example, this branch is for adding days to date start/end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// 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
Comment on lines +2621 to +2625
Copy link
Contributor Author

@TJX2014 TJX2014 Jul 6, 2020

Choose a reason for hiding this comment

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

@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.

backedSequenceImpl.eval(start, stop, fromLong(stepMicros))

} else {
Expand Down Expand Up @@ -2674,11 +2681,24 @@ object Sequence {
|${genSequenceLengthCode(ctx, startMicros, stopMicros, intervalInMicros, arrLength)}
""".stripMargin

val check = if (scale == MICROS_PER_DAY) {
s"""
|if ($stepMonths == 0 && $stepDays == 0) {
| throw new IllegalArgumentException(
| "sequence step must be a day interval if start and end values are dates");
|}
""".stripMargin
} else {
""
}

s"""
|final int $stepMonths = $step.months;
|final int $stepDays = $step.days;
|final long $stepMicros = $step.microseconds;
|
|$check
|
|if ($stepMonths == 0 && $stepMicros == 0 && ${scale}L == ${MICROS_PER_DAY}L) {
| ${backedSequenceImpl.genCode(ctx, start, stop, stepDays, arr, elemType)};
|
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -933,6 +933,13 @@ class CollectionExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper
Literal(negateExact(stringToInterval("interval 1 month")))),
EmptyRow,
s"sequence boundaries: 0 to 2678400000000 by -${28 * MICROS_PER_DAY}")

// 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")
}
}

Expand Down