[MINOR][DOC] Fix the CalendarIntervalType description #26659
Closed
Conversation
This file contains hidden or 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
Member
Author
|
cc @cloud-fan, please take a look at this, I believe this is trivial but important. |
|
Test build #114383 has finished for PR 26659 at commit
|
cloud-fan
approved these changes
Nov 25, 2019
|
Test build #114397 has finished for PR 26659 at commit
|
Contributor
|
|
MaxGekk
reviewed
Nov 25, 2019
Member
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
Should we say what is the range of valid values. DateType and TimestampType have min and max valid points. Can the INTERVAL type represents interval greater than max-min timestamp?
| import org.apache.spark.unsafe.types.CalendarInterval | ||
|
|
||
| /** | ||
| * The data type representing calendar time intervals. The calendar time interval is stored |
Member
There was a problem hiding this comment.
representing -> represents? @srowen Does representing sound good for you?
Member
There was a problem hiding this comment.
representing sounds right. "[This class is] The data type representing ..."
| * The data type representing calendar time intervals. The calendar time interval is stored | ||
| * internally in two components: number of months the number of microseconds. | ||
| * internally in three components: | ||
| * the number of months |
Member
There was a problem hiding this comment.
I think we should say explicitly that the numbers can be positive as well as negative
| import org.apache.spark.unsafe.types.CalendarInterval | ||
|
|
||
| /** | ||
| * The data type representing calendar time intervals. The calendar time interval is stored |
Member
There was a problem hiding this comment.
calendar time intervals . It is arguable but it can represent date interval too.
|
Test build #114413 has finished for PR 26659 at commit
|
gengliangwang
approved these changes
Nov 26, 2019
Contributor
|
thanks, merging to master! |
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 <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
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 <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 7df4fed) Signed-off-by: Wenchen Fan <[email protected]>
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 <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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?
fix the overdue and incorrect description about CalendarIntervalType
Why are the changes needed?
api doc correctness
Does this PR introduce any user-facing change?
no
How was this patch tested?
no