[SPARK-30551][SQL] Disable comparison for interval type #27262
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cloud-fan
reviewed
Jan 17, 2020
| @@ -29,7 +29,7 @@ | |||
| /** | |||
| * The internal representation of interval type. | |||
| */ | |||
| public final class CalendarInterval implements Serializable, Comparable<CalendarInterval> { | |||
| public final class CalendarInterval implements Serializable { | |||
There was a problem hiding this comment.
there is a MAX_VALUE and MIN_VALUE in this file, shall we remove them as well?
cloud-fan
reviewed
Jan 17, 2020
| protected var upper: CalendarInterval = CalendarInterval.MIN_VALUE | ||
| protected var lower: CalendarInterval = CalendarInterval.MAX_VALUE | ||
| protected var upper: CalendarInterval = null | ||
| protected var lower: CalendarInterval = null |
There was a problem hiding this comment.
can we follow BinaryColumnStats and don't define the upper and lower variable?
|
shall we also revert #26681 ? |
thanks, I missed this one |
|
Test build #116944 has finished for PR 27262 at commit
|
|
Test build #116937 has finished for PR 27262 at commit
|
cloud-fan
approved these changes
Jan 17, 2020
dongjoon-hyun
requested changes
Jan 17, 2020
There was a problem hiding this comment.
Please be clear for the reverting in the PR description, @yaooqinn .
If SPARK-30048 is clearly reverted by this PR, could you update SPARK-30048 JIRA, @cloud-fan ?
|
Also, cc @maropu |
|
Test build #116954 has finished for PR 27262 at commit
|
|
I've updated SPARK-29679 and SPARK-30048 as won't do and mention they have been reverted. Thanks, merging to master! |
|
Thank you for the clarification, @cloud-fan and @yaooqinn . |
|
Checked; late LGTM. |
cloud-fan
pushed a commit
that referenced
this pull request
Feb 22, 2021
…lity ### What changes were proposed in this pull request? In the PR, I propose to revert #26659 partially regarding to comparability of interval values. The comment became incorrect after #27262. ### Why are the changes needed? The comment is incorrect, and it might confuse Spark's devs/users. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By checking scala coding style `./dev/scalastyle`. Closes #31610 from MaxGekk/doc-interval-not-comparable. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan
pushed a commit
that referenced
this pull request
Feb 22, 2021
…lity ### What changes were proposed in this pull request? In the PR, I propose to revert #26659 partially regarding to comparability of interval values. The comment became incorrect after #27262. ### Why are the changes needed? The comment is incorrect, and it might confuse Spark's devs/users. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By checking scala coding style `./dev/scalastyle`. Closes #31610 from MaxGekk/doc-interval-not-comparable. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 7df4fed) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
a0x8o
added a commit
to a0x8o/spark
that referenced
this pull request
Feb 22, 2021
…lity ### What changes were proposed in this pull request? In the PR, I propose to revert apache/spark#26659 partially regarding to comparability of interval values. The comment became incorrect after apache/spark#27262. ### Why are the changes needed? The comment is incorrect, and it might confuse Spark's devs/users. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By checking scala coding style `./dev/scalastyle`. Closes #31610 from MaxGekk/doc-interval-not-comparable. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
As we are not going to follow ANSI to implement year-month and day-time interval types, it is weird to compare the year-month part to the day-time part for our current implementation of interval type now.
Additionally, the current ordering logic comes from PostgreSQL where the implementation of the interval is messy. And we are not aiming PostgreSQL compliance at all.
THIS PR will revert #26681 and #26337
Why are the changes needed?
make interval type more future-proofing
Does this PR introduce any user-facing change?
there are new in 3.0, so no
How was this patch tested?
existing uts shall work