Skip to content
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

[SPARK-26517][SQL][TEST] Avoid duplicate test in ParquetSchemaPruningSuite #23427

Closed
wants to merge 1 commit into from
Closed

[SPARK-26517][SQL][TEST] Avoid duplicate test in ParquetSchemaPruningSuite #23427

wants to merge 1 commit into from

Conversation

viirya
Copy link
Member

@viirya viirya commented Jan 2, 2019

What changes were proposed in this pull request?

testExactCaseQueryPruning and testMixedCaseQueryPruning don't need to set up PARQUET_VECTORIZED_READER_ENABLED config. Because withMixedCaseData will run against both Spark vectorized reader and Parquet-mr reader.

How was this patch tested?

Existing test.

@viirya
Copy link
Member Author

viirya commented Jan 2, 2019

cc @dbtsai @mallman @cloud-fan

@SparkQA
Copy link

SparkQA commented Jan 2, 2019

Test build #100636 has finished for PR 23427 at commit cb67777.

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

@mallman
Copy link
Contributor

mallman commented Jan 2, 2019

@viirya I don't see how withMixedCaseData tests with both readers. Can you explain more?

dongjoon-hyun
dongjoon-hyun approved these changes Jan 2, 2019
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 2, 2019 •
edited
Loading

@mallman . Please see withParquetTable, withParquetDataFrame and readParquetFile.

@mallman
Copy link
Contributor

mallman commented Jan 2, 2019

Please see withParquetTable, withParquetDataFrame and readParquetFile.

I see. I'm still concerned that this may not be doing what it appears to be doing. I had to refactor the location of the SQLConf settings in a previous commit because they weren't working as intended.

Please give me a day or two to respond after I've had the opportunity to do some additional verification of my own. I cannot get right to this because of my schedule.

@mallman
Copy link
Contributor

mallman commented Jan 2, 2019

My concerns were founded around the problem fixed by #22394, which appears to be unrelated. Based on that, I will endorse this PR with a +1. Thank you, @viirya for your diligence.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 2, 2019 •
edited
Loading

Ur, @mallman . So, you will endorse later?

I was just confused. No problem with waiting for your review. :)

HyukjinKwon
HyukjinKwon approved these changes Jan 3, 2019
View reviewed changes
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.

LGTM

@srowen
Copy link
Member

srowen commented Jan 3, 2019

Merged to master

@srowen srowen closed this in 40711ee Jan 3, 2019
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@viirya @jackylee-ch
[SPARK-26517][SQL][TEST] Avoid duplicate test in ParquetSchemaPruning…
151b94a 
…Suite

## What changes were proposed in this pull request?

`testExactCaseQueryPruning` and `testMixedCaseQueryPruning` don't need to set up `PARQUET_VECTORIZED_READER_ENABLED` config. Because `withMixedCaseData` will run against both Spark vectorized reader and Parquet-mr reader.

## How was this patch tested?

Existing test.

Closes apache#23427 from viirya/fix-parquet-schema-pruning-test.

Authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Signed-off-by: Sean Owen <sean.owen@databricks.com>
@viirya viirya deleted the fix-parquet-schema-pruning-test branch December 27, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srowen srowen srowen approved these changes

@cloud-fan cloud-fan cloud-fan approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@viirya @SparkQA @mallman @dongjoon-hyun @srowen @cloud-fan @HyukjinKwon