Skip to content

[SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence #28926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TJX2014 wants to merge 12 commits into from
Closed

[SPARK-32133][SQL] Forbid time field steps for date start/end in Sequence #28926

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
665ac46
forbid time field steps for date start/end
TJX2014 Jun 29, 2020
7733184
SPARK-32198 -> SPARK-32133
TJX2014 Jun 29, 2020
74c683f
improve code
TJX2014 Jun 30, 2020
9f910d7
move test case to Sequence of dates
TJX2014 Jun 30, 2020
9857fdc
format improve
TJX2014 Jul 2, 2020
b0eb77f
code style improve
TJX2014 Jul 2, 2020
df2138c
check position indentation
TJX2014 Jul 2, 2020
b863eef
remove 2 space indentation
TJX2014 Jul 3, 2020
9a521a3
space correct
TJX2014 Jul 5, 2020
2bd2d0b
Merge branch 'master-SPARK-31982-sequence-cross-dst-follow-presto' of…
TJX2014 Jul 5, 2020
d860c0e
new code comment to date and timestamp
TJX2014 Jul 6, 2020
8c4ffaa
Merge branch 'master-SPARK-31982-sequence-cross-dst-follow-presto' of…
TJX2014 Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2618,9 +2618,11 @@ object Sequence {
}

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

@cloud-fan cloud-fan Jun 29, 2020

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

@TJX2014 TJX2014 Jun 29, 2020

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

@dongjoon-hyun dongjoon-hyun Jul 5, 2020

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

@dongjoon-hyun dongjoon-hyun Jul 5, 2020

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 •
edited
Loading

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

@cloud-fan cloud-fan Jul 6, 2020

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

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

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 •
edited
Loading

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