Skip to content

[SPARK-25406][SQL] For ParquetSchemaPruningSuite.scala, move calls to withSQLConf inside calls to test #22394

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

Closed
mallman wants to merge 3 commits into from
Closed

[SPARK-25406][SQL] For ParquetSchemaPruningSuite.scala, move calls to withSQLConf inside calls to test #22394

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cca76b
For ParquetSchemaPruningSuite.scala, move calls to `withSQLConf` inside
mallman Sep 11, 2018
c759aea
We shouldn't expect queries selecting columns using different case to
mallman Sep 11, 2018
1c89637
Clarify the intent of the test methods for case-insensitive pruning
mallman Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions .../scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaPruningSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,20 +156,24 @@ class ParquetSchemaPruningSuite
}

private def testSchemaPruning(testName: String)(testThunk: => Unit) {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true") {
test(s"Spark vectorized reader - without partition data column - $testName") {
test(s"Spark vectorized reader - without partition data column - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true") {
Comment thread
mallman marked this conversation as resolved.
withContacts(testThunk)
}
test(s"Spark vectorized reader - with partition data column - $testName") {
}
test(s"Spark vectorized reader - with partition data column - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true") {
withContactsWithDataPartitionColumn(testThunk)
}
}

withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false") {
test(s"Parquet-mr reader - without partition data column - $testName") {
test(s"Parquet-mr reader - without partition data column - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false") {
withContacts(testThunk)
}
test(s"Parquet-mr reader - with partition data column - $testName") {
}
test(s"Parquet-mr reader - with partition data column - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false") {
withContactsWithDataPartitionColumn(testThunk)
}
}
Expand Down Expand Up @@ -209,7 +213,7 @@ class ParquetSchemaPruningSuite
MixedCase(1, "r1c1", MixedCaseColumn("123", 2)) ::
Nil

testMixedCasePruning("select with exact column names") {
testExactCasePruning("select with exact column names") {
val query = sql("select CoL1, coL2.B from mixedcase")
checkScan(query, "struct<CoL1:string,coL2:struct<B:int>>")
checkAnswer(query.orderBy("id"),
Expand Down Expand Up @@ -245,28 +249,32 @@ class ParquetSchemaPruningSuite
checkAnswer(query.orderBy("id"), Row(1) :: Nil)
}

private def testMixedCasePruning(testName: String)(testThunk: => Unit) {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true",
SQLConf.CASE_SENSITIVE.key -> "true") {
test(s"Spark vectorized reader - case-sensitive parser - mixed-case schema - $testName") {
withMixedCaseData(testThunk)
private def testExactCasePruning(testName: String)(testThunk: => Unit) {

@viirya viirya Sep 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

testCaseSensitivePruning?

@HyukjinKwon HyukjinKwon Sep 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks testing case insensitivity too?

@mallman mallman Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The method names are meant to clarify the kind of queries being tested, not the setting of SQLConf.CASE_SENSITIVE.key. In this case, testExactCasePruning is supposed to mean that we're passing in a test in which the column names in the query are exactly the same as the column names in the relation.

It's not a very good name in that sense. I'll try to make it clearer and add a code comment to clarify.

test(s"Spark vectorized reader - case-sensitive parser - mixed-case schema - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true",
SQLConf.CASE_SENSITIVE.key -> "true") {
withMixedCaseData(testThunk)
}
}
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false",
SQLConf.CASE_SENSITIVE.key -> "false") {
test(s"Parquet-mr reader - case-insensitive parser - mixed-case schema - $testName") {
test(s"Parquet-mr reader - case-sensitive parser - mixed-case schema - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false",
SQLConf.CASE_SENSITIVE.key -> "true") {
withMixedCaseData(testThunk)
}
}
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true",
SQLConf.CASE_SENSITIVE.key -> "false") {
test(s"Spark vectorized reader - case-insensitive parser - mixed-case schema - $testName") {
withMixedCaseData(testThunk)
testMixedCasePruning(testName)(testThunk)
}

private def testMixedCasePruning(testName: String)(testThunk: => Unit) {

@viirya viirya Sep 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

testCaseInSensitivePruning?

@HyukjinKwon HyukjinKwon Sep 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

testMixedCasePruning looks previously testing case sensitive case too. Now, it looks not. Would you mind if I ask the reason why?

@mallman mallman Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previously, this method ran testThunk with SQLConf.CASE_SENSITIVE.key set to true and false. That was a mistake and incorrect. For example, the query

select col1, col2.b from mixedcase

will fail if SQLConf.CASE_SENSITIVE.key is set to true. That mistake was causing 6 test cases to fail. Therefore, I moved the code that tests with a case-sensitive parser out of testMixedCasePruning into testExactCasePruning and included a call to testMixedCasePruning in testExactCasePruning.

I'll push a commit that refactors the method names and add code comments that will make this clearer.

test(s"Parquet-mr reader - case-insensitive parser - mixed-case schema - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false",

@viirya viirya Sep 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The vectorized reader cases in both testSchemaPruning and testExactCasePruning are put ahead of Parquet-mr reader cases. Shall we follow it too in testMixedCasePruning?

SQLConf.CASE_SENSITIVE.key -> "false") {
withMixedCaseData(testThunk)
}
}
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false",
SQLConf.CASE_SENSITIVE.key -> "true") {
test(s"Parquet-mr reader - case-sensitive parser - mixed-case schema - $testName") {
test(s"Spark vectorized reader - case-insensitive parser - mixed-case schema - $testName") {
withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "true",
SQLConf.CASE_SENSITIVE.key -> "false") {
withMixedCaseData(testThunk)
}
}
Expand Down