Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented May 21, 2020

What changes were proposed in this pull request?

Add withAllParquetReaders to ParquetTest. The function allow to run a block of code for all available Parquet readers.

Why are the changes needed?

  1. It simplifies tests
  2. Allow to test all parquet readers that could be available in projects based on Apache Spark.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running affected test suites.

@MaxGekk
Copy link
Member Author

MaxGekk commented May 21, 2020

@cloud-fan @mswit-databricks @adrian-ionescu Please, review this PR.

withParquetReaders("parquet-mr", "vectorized-oss")(code)
}

def withAllParquetReaders(code: => Unit): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we simplify it?

def withAllParquetReaders(code: => Unit): Unit = {
  // test the row-based reader
  withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false")(code)
  // test the vectorized reader
  withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true")(code)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I would say code- > func but no big deal.

@SparkQA
Copy link

SparkQA commented May 21, 2020

Test build #122930 has finished for PR 28598 at commit 1c8b0e9.

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

@SparkQA
Copy link

SparkQA commented May 21, 2020

Test build #122936 has finished for PR 28598 at commit 5ffc1e4.

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

@HyukjinKwon
Copy link
Member

Merged to master and branch-3.0.

HyukjinKwon pushed a commit that referenced this pull request May 22, 2020
…eaders

### What changes were proposed in this pull request?
Add `withAllParquetReaders` to `ParquetTest`. The function allow to run a block of code for all available Parquet readers.

### Why are the changes needed?
1. It simplifies tests
2. Allow to test all parquet readers that could be available in projects based on Apache Spark.

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

### How was this patch tested?
By running affected test suites.

Closes #28598 from MaxGekk/add-withAllParquetReaders.

Authored-by: Max Gekk <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
(cherry picked from commit 60118a2)
Signed-off-by: HyukjinKwon <[email protected]>
@MaxGekk MaxGekk deleted the add-withAllParquetReaders branch June 5, 2020 19:44
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