Skip to content

[SPARK-31476][SQL][FOLLOWUP] Add tests for extract('field', source) #28276

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[SPARK-31476][SQL][FOLLOWUP] Add tests for extract('field', source) #28276

wants to merge 2 commits into from

Conversation

@maropu
Copy link
Member

@maropu maropu commented Apr 21, 2020

What changes were proposed in this pull request?

SPARK-31476 has supported extract('field', source) as side-effect, so this PR intends to add some tests for the function in SQLQueryTestSuite.

Why are the changes needed?

For better test coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added tests.

maropu
maropu commented Apr 21, 2020
View reviewed changes
sql/core/src/test/resources/sql-tests/inputs/extract.sql Outdated
select extract(not_supported from i) from t;

-- In SPARK-31476, we've supported extract('field', source), too
select date_part('millennium', c) from t;
Copy link
Member Author

@maropu maropu Apr 21, 2020

Choose a reason for hiding this comment

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

I added only simple test patterns for extract('field', source).

Copy link
Member

@HyukjinKwon HyukjinKwon Apr 21, 2020

Choose a reason for hiding this comment

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

I think only a couple of tests are fine :-).

Copy link
Member

@HyukjinKwon HyukjinKwon Apr 21, 2020

Choose a reason for hiding this comment

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

BTW, why is it date_part instead of extract? date_part seems being tested at date_part.sql.

Copy link
Member

@HyukjinKwon HyukjinKwon Apr 21, 2020

Choose a reason for hiding this comment

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

Wait .. are DatePart and Extract the same? If so, it might be better to have one class and handle aliases via expression(..., setAlias=true) at FunctionRegistry.

Copy link
Member Author

@maropu maropu Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, this support (extract('field', source)) is a side-effect of #31476.... EXTRACT should be formed as EXTRACT(field FROM source), so the example field in ExpressionDescription is different now from each other. Related discussion: #21479 (comment) cc: @cloud-fan

Copy link
Member

@HyukjinKwon HyukjinKwon Apr 21, 2020

Choose a reason for hiding this comment

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

I see. +1 to have a separate examples and documentation.

Copy link
Contributor

@cloud-fan cloud-fan Apr 21, 2020

Choose a reason for hiding this comment

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

I think the side effect is we support extract('field', source), why we are testing date_part here...

Copy link
Member Author

@maropu maropu Apr 21, 2020

Choose a reason for hiding this comment

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

Ur, my bad... I'll update soon...

Copy link
Member Author

@maropu maropu Apr 21, 2020

Choose a reason for hiding this comment

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

Updated.

@maropu maropu mentioned this pull request Apr 21, 2020
Closed
@maropu
Copy link
Member Author

maropu commented Apr 21, 2020

cc: @cloud-fan @dongjoon-hyun

@SparkQA
Copy link

SparkQA commented Apr 21, 2020

Test build #121551 has finished for PR 28276 at commit fb3acc1.

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

@SparkQA
Copy link

SparkQA commented Apr 21, 2020

Test build #121577 has finished for PR 28276 at commit cf9a9ea.

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

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Apr 21, 2020

Although this comes first, shall we wait for #28284 ? That will remove some support fields. So, it will reduce the number of test cases here.

@maropu
Copy link
Member Author

maropu commented Apr 22, 2020

sure, thanks, @dongjoon-hyun

@cloud-fan
Copy link
Contributor

cloud-fan commented Apr 22, 2020

#28284 has been merged, let's update the tests here

@maropu maropu force-pushed the SPARK-31476-FOLLOWUP branch from cf9a9ea to 04642b3 Compare April 22, 2020 12:07
@maropu
Copy link
Member Author

maropu commented Apr 22, 2020

ok, udpated.

cloud-fan
cloud-fan reviewed Apr 22, 2020
View reviewed changes
sql/core/src/test/resources/sql-tests/results/extract.sql.out Outdated
struct<>
-- !query output
org.apache.spark.sql.AnalysisException
Literals of type 'millennium' are currently not supported for the string type.;; line 1 pos 7
Copy link
Contributor

@cloud-fan cloud-fan Apr 22, 2020

Choose a reason for hiding this comment

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

let's remove these unsupported tests

@maropu maropu force-pushed the SPARK-31476-FOLLOWUP branch from f40578d to 28758ae Compare April 22, 2020 15:23
@SparkQA
Copy link

SparkQA commented Apr 22, 2020

Test build #121619 has finished for PR 28276 at commit 04642b3.

  • 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 #121631 has finished for PR 28276 at commit 28758ae.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Apr 23, 2020

thanks, merging to master/3.0!

@cloud-fan cloud-fan closed this in 820733a Apr 23, 2020
cloud-fan pushed a commit that referenced this pull request Apr 23, 2020
@maropu @cloud-fan
[SPARK-31476][SQL][FOLLOWUP] Add tests for extract('field', source)
6e76310 
### What changes were proposed in this pull request?

SPARK-31476 has supported `extract('field', source)` as side-effect, so this PR intends to add some tests for the function in `SQLQueryTestSuite`.

### Why are the changes needed?

For better test coverage.

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

No.

### How was this patch tested?

Added tests.

Closes #28276 from maropu/SPARK-31476-FOLLOWUP.

Authored-by: Takeshi Yamamuro <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 820733a)
Signed-off-by: Wenchen Fan <[email protected]>
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Apr 23, 2020

+1, late LGTM. Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maropu @SparkQA @dongjoon-hyun @cloud-fan @HyukjinKwon