Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Apr 22, 2020

What changes were proposed in this pull request?

  • Document row field values of DATE and TIMESTAMP type returned by Row.get() and Row.apply.
  • Refer to Row.get() from the description of filter values

Why are the changes needed?

Reflect current behaviour of Row's method apply() and get() in comments to inform users about different return types that are depended on the SQL config settings spark.sql.datetime.java8API.enabled.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Run $ ./dev/scalastyle

@MaxGekk
Copy link
Member Author

MaxGekk commented Apr 22, 2020

@cloud-fan @HyukjinKwon Please, review the PR.

@MaxGekk
Copy link
Member Author

MaxGekk commented Apr 22, 2020

The comment describes the default case when Java 8 date-time API is off, and it will be absolutely correct after the PR #28272 is merged.

@MaxGekk MaxGekk changed the title [MINOR][SQL] Add a comment for date/timestamp filter values [MINOR][SQL] Add comments for filters values and return values of Row.get()/apply() Apr 22, 2020
@SparkQA
Copy link

SparkQA commented Apr 22, 2020

Test build #121623 has finished for PR 28300 at commit c4dda25.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 22, 2020

Test build #121637 has finished for PR 28300 at commit 4ac780e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

To me, looks fine but let me leave it to @cloud-fan

@cloud-fan
Copy link
Contributor

thanks, merging to master/3.0!

@cloud-fan cloud-fan closed this in e7856a7 Apr 23, 2020
cloud-fan pushed a commit that referenced this pull request Apr 23, 2020
….get()/apply()

### What changes were proposed in this pull request?
- Document row field values of `DATE` and `TIMESTAMP` type returned by `Row.get()` and `Row.apply`.
- Refer to `Row.get()` from the description of filter values

### Why are the changes needed?
Reflect current behaviour of Row's method `apply()` and `get()` in comments to inform users about different return types that are depended on the SQL config settings `spark.sql.datetime.java8API.enabled`.

### Does this PR introduce any user-facing change?
No

### How was this patch tested?
Run `$ ./dev/scalastyle`

Closes #28300 from MaxGekk/doc-filter-date-time.

Authored-by: Max Gekk <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit e7856a7)
Signed-off-by: Wenchen Fan <[email protected]>
@MaxGekk MaxGekk deleted the doc-filter-date-time branch June 5, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants