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

mallman · 2018-09-11T15:18:50Z

(Link to Jira: https://issues.apache.org/jira/browse/SPARK-25406)

What changes were proposed in this pull request?

The current use of withSQLConf in ParquetSchemaPruningSuite.scala is incorrect. The desired configuration settings are not being set when running the test cases.

This PR fixes that defective usage and addresses the test failures that were previously masked by that defect.

How was this patch tested?

I added code to relevant test cases to print the expected SQL configuration settings and found that the settings were not being set as expected. When I changed the order of calls to test and withSQLConf I found that the configuration settings were being set as expected.

calls to `test`

mallman · 2018-09-11T15:24:10Z

I'm working on fixing these test failures now. Hopefully I'll have something pushed soon.

succeed when using a case-sensitive query parser

mallman · 2018-09-11T15:45:47Z

FYI @viirya @dbtsai @gatorsmile @HyukjinKwon

Can I get someone's review of this PR please? The unmasked failures appear to be false positives, so no changes to the tested code are required—just changes to the tests themselves.

Thanks.

SparkQA · 2018-09-11T17:26:30Z

Test build #95951 has finished for PR 22394 at commit 8cca76b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-09-11T17:34:46Z

Test build #95952 has finished for PR 22394 at commit c759aea.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-09-12T01:22:46Z

retest this please.

...est/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaPruningSuite.scala

viirya · 2018-09-12T01:46:12Z

...est/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaPruningSuite.scala

+
+  private def testMixedCasePruning(testName: String)(testThunk: => Unit) {
+    test(s"Parquet-mr reader - case-insensitive parser - mixed-case schema - $testName") {
+      withSQLConf(SQLConf.PARQUET_VECTORIZED_READER_ENABLED.key -> "false",


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

viirya · 2018-09-12T01:47:42Z

...est/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaPruningSuite.scala

-      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) {


testCaseSensitivePruning?

This looks testing case insensitivity too?

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.

viirya · 2018-09-12T01:48:04Z

...est/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaPruningSuite.scala

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


testCaseInSensitivePruning?

SparkQA · 2018-09-12T04:57:25Z

Test build #95970 has finished for PR 22394 at commit c759aea.

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

HyukjinKwon · 2018-09-12T05:51:25Z

...est/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaPruningSuite.scala

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


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

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.

HyukjinKwon · 2018-09-12T05:51:57Z

Hey @mallman, let's just target to fix the problem in the JIRA without other refactorings.

testing

mallman · 2018-09-12T14:52:03Z

Hey @mallman, let's just target to fix the problem in the JIRA without other refactorings.

The refactorings I've made address the problem directly. Hopefully that will be clearer with my most recent commit.

SparkQA · 2018-09-12T16:41:07Z

Test build #95992 has finished for PR 22394 at commit 1c89637.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mallman · 2018-09-12T16:52:38Z

Retest this please.

SparkQA · 2018-09-12T18:46:07Z

Test build #95997 has finished for PR 22394 at commit 1c89637.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-09-13T03:40:18Z

retest this please

SparkQA · 2018-09-13T07:05:01Z

Test build #96018 has finished for PR 22394 at commit 1c89637.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dilipbiswal · 2018-09-13T07:08:23Z

retest this please

SparkQA · 2018-09-13T10:58:34Z

Test build #96029 has finished for PR 22394 at commit 1c89637.

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

dbtsai · 2018-09-13T17:05:26Z

LGTM. Merged into master and branch 2.4. Thanks.

… `withSQLConf` inside calls to `test` (Link to Jira: https://issues.apache.org/jira/browse/SPARK-25406) ## What changes were proposed in this pull request? The current use of `withSQLConf` in `ParquetSchemaPruningSuite.scala` is incorrect. The desired configuration settings are not being set when running the test cases. This PR fixes that defective usage and addresses the test failures that were previously masked by that defect. ## How was this patch tested? I added code to relevant test cases to print the expected SQL configuration settings and found that the settings were not being set as expected. When I changed the order of calls to `test` and `withSQLConf` I found that the configuration settings were being set as expected. Closes #22394 from mallman/spark-25406-fix_broken_schema_pruning_tests. Authored-by: Michael Allman <msa@allman.ms> Signed-off-by: DB Tsai <d_tsai@apple.com> (cherry picked from commit a7e5aa6) Signed-off-by: DB Tsai <d_tsai@apple.com>

… `withSQLConf` inside calls to `test` (Link to Jira: https://issues.apache.org/jira/browse/SPARK-25406) The current use of `withSQLConf` in `ParquetSchemaPruningSuite.scala` is incorrect. The desired configuration settings are not being set when running the test cases. This PR fixes that defective usage and addresses the test failures that were previously masked by that defect. I added code to relevant test cases to print the expected SQL configuration settings and found that the settings were not being set as expected. When I changed the order of calls to `test` and `withSQLConf` I found that the configuration settings were being set as expected. Closes apache#22394 from mallman/spark-25406-fix_broken_schema_pruning_tests. Authored-by: Michael Allman <msa@allman.ms> Signed-off-by: DB Tsai <d_tsai@apple.com> (cherry picked from commit a7e5aa6) Signed-off-by: DB Tsai <d_tsai@apple.com> Ref: LIHADOOP-48531 RB=1857521 A=

For ParquetSchemaPruningSuite.scala, move calls to withSQLConf inside

8cca76b

calls to `test`

mallman mentioned this pull request Sep 11, 2018

[SPARK-25363][SQL] Fix schema pruning in where clause by ignoring unnecessary root fields #22357

Closed

We shouldn't expect queries selecting columns using different case to

c759aea

succeed when using a case-sensitive query parser

viirya reviewed Sep 12, 2018

View reviewed changes

...est/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaPruningSuite.scala Show resolved Hide resolved

viirya reviewed Sep 12, 2018

View reviewed changes

HyukjinKwon reviewed Sep 12, 2018

View reviewed changes

Clarify the intent of the test methods for case-insensitive pruning

1c89637

testing

asfgit closed this in a7e5aa6 Sep 13, 2018

mallman mentioned this pull request Jan 2, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mallman commented Sep 11, 2018

mallman commented Sep 11, 2018

mallman commented Sep 11, 2018

SparkQA commented Sep 11, 2018

SparkQA commented Sep 11, 2018

viirya commented Sep 12, 2018

viirya Sep 12, 2018

viirya Sep 12, 2018

HyukjinKwon Sep 12, 2018

mallman Sep 12, 2018

viirya Sep 12, 2018

SparkQA commented Sep 12, 2018

HyukjinKwon Sep 12, 2018

mallman Sep 12, 2018

HyukjinKwon commented Sep 12, 2018

mallman commented Sep 12, 2018

SparkQA commented Sep 12, 2018

mallman commented Sep 12, 2018

SparkQA commented Sep 12, 2018

HyukjinKwon commented Sep 13, 2018

SparkQA commented Sep 13, 2018

dilipbiswal commented Sep 13, 2018

SparkQA commented Sep 13, 2018

dbtsai commented Sep 13, 2018

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

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

Conversation

mallman commented Sep 11, 2018

What changes were proposed in this pull request?

How was this patch tested?

mallman commented Sep 11, 2018

mallman commented Sep 11, 2018

SparkQA commented Sep 11, 2018

SparkQA commented Sep 11, 2018

viirya commented Sep 12, 2018

viirya Sep 12, 2018

Choose a reason for hiding this comment

viirya Sep 12, 2018

Choose a reason for hiding this comment

HyukjinKwon Sep 12, 2018

Choose a reason for hiding this comment

mallman Sep 12, 2018

Choose a reason for hiding this comment

viirya Sep 12, 2018

Choose a reason for hiding this comment

SparkQA commented Sep 12, 2018

HyukjinKwon Sep 12, 2018

Choose a reason for hiding this comment

mallman Sep 12, 2018

Choose a reason for hiding this comment

HyukjinKwon commented Sep 12, 2018

mallman commented Sep 12, 2018

SparkQA commented Sep 12, 2018

mallman commented Sep 12, 2018

SparkQA commented Sep 12, 2018

HyukjinKwon commented Sep 13, 2018

SparkQA commented Sep 13, 2018

dilipbiswal commented Sep 13, 2018

SparkQA commented Sep 13, 2018

dbtsai commented Sep 13, 2018

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

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