[SPARK-8690][SQL] Add a setting to disable SparkSQL parquet schema merge by using datasource API #7070

thegiive · 2015-06-28T10:45:06Z

The detail problem story is in https://issues.apache.org/jira/browse/SPARK-8690

General speaking, I add a config spark.sql.parquet.mergeSchema to achieve the sqlContext.load("parquet" , Map( "path" -> "..." , "mergeSchema" -> "false" ))

It will become a simple flag and without any side affect.

…tasource API schema merge feature.

AmplabJenkins · 2015-06-28T10:47:12Z

Can one of the admins verify this patch?

liancheng · 2015-06-29T23:01:25Z

ok to test

AmplabJenkins · 2015-06-29T23:02:13Z

Merged build triggered.

AmplabJenkins · 2015-06-29T23:02:21Z

Merged build started.

SparkQA · 2015-06-29T23:05:45Z

Test build #36048 has started for PR 7070 at commit b6806fb.

SparkQA · 2015-06-29T23:07:39Z

Test build #36048 has finished for PR 7070 at commit b6806fb.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-06-29T23:08:01Z

Merged build finished. Test PASSed.

liancheng · 2015-06-29T23:56:57Z

sql/core/src/main/scala/org/apache/spark/sql/parquet/newParquet.scala

@@ -114,7 +114,7 @@ private[sql] class ParquetRelation2(

  // Should we merge schemas from all Parquet part-files?
  private val shouldMergeSchemas =
-    parameters.getOrElse(ParquetRelation2.MERGE_SCHEMA, "true").toBoolean
+    parameters.getOrElse(ParquetRelation2.MERGE_SCHEMA, sqlContext.getConf("spark.sql.parquet.mergeSchema" , "true") ).toBoolean


Several styling issue here:

100 columns exceeded

Remove the space before ,

Remove the space before )

liancheng · 2015-06-29T23:58:44Z

Thanks for contributing this! This feature looks good, but still requires some more polishing:

Please fix the style issues as I commented.
Please add this configuration to SQLConf. Example:
- SQLConf.PARQUET_FILTER_PUSHDOWN_ENABLED
- SQLConf.parquetFilterPushDown
Please add a test case for this.

For example, we can write two Parquet files with incompatible schemas and put them into the same directory, then read the directory to see whether the schema merging is triggered (if it's triggered, exception will be thrown due to incompatible schemas).

thegiive · 2015-06-30T22:41:57Z

Sure. I have already finish the coding. But I need some more time to write the test case

AmplabJenkins · 2015-07-01T12:18:11Z

Merged build triggered.

AmplabJenkins · 2015-07-01T12:20:06Z

Merged build started.

SparkQA · 2015-07-01T12:24:37Z

Test build #36247 has started for PR 7070 at commit db8ef1b.

SparkQA · 2015-07-01T12:28:00Z

Test build #36247 has finished for PR 7070 at commit db8ef1b.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-07-01T12:28:01Z

Merged build finished. Test FAILed.

AmplabJenkins · 2015-07-01T12:43:12Z

Merged build triggered.

thegiive · 2015-07-01T12:43:12Z

Hi @liancheng , Can you help to check if it is what you suggest ?

AmplabJenkins · 2015-07-01T12:43:18Z

Merged build started.

SparkQA · 2015-07-01T12:44:19Z

Test build #36259 has started for PR 7070 at commit 94c9307.

SparkQA · 2015-07-01T14:17:50Z

Test build #36259 has finished for PR 7070 at commit 94c9307.

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

AmplabJenkins · 2015-07-01T14:18:23Z

Merged build finished. Test PASSed.

liancheng · 2015-07-01T17:54:20Z

sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala

@@ -227,6 +227,13 @@ private[spark] object SQLConf {
    defaultValue = Some(true),
    doc = "<TODO>")

+  val PARQUET_MERGE_SCHEMA_ENABLED = booleanConf("spark.sql.parquet.mergeSchema",


PARQUET_SCHEMA_MERGING_ENABLED might be a better name.

liancheng · 2015-07-01T17:54:28Z

sql/core/src/test/scala/org/apache/spark/sql/parquet/ParquetSchemaMergeConfigSuite.scala

+
+
+class ParquetSchemaMergeConfigSuite  extends QueryTest with ParquetTest with BeforeAndAfterAll {
+  val sqlContext = TestSQLContext


Please add override.

liancheng · 2015-07-01T17:59:32Z

@thegiive I left several comments on styling issues, otherwise your changes looks pretty good to me now. The Databricks Scala style guide and Spark code style guide can be good references for code styling.

liancheng · 2015-07-01T18:39:54Z

sql/core/src/test/scala/org/apache/spark/sql/parquet/ParquetSchemaMergeConfigSuite.scala

+
+import org.apache.spark.sql.{SQLConf, QueryTest}
+import org.apache.spark.sql.test.TestSQLContext
+import org.scalatest.BeforeAndAfterAll


Reorder imports. Please refer to https://github.com/databricks/scala-style-guide#imports

…eConfigSuite

AmplabJenkins · 2015-07-01T22:33:11Z

Merged build triggered.

AmplabJenkins · 2015-07-01T22:33:22Z

Merged build started.

SparkQA · 2015-07-01T22:36:38Z

Test build #36310 has started for PR 7070 at commit c6f3e86.

SparkQA · 2015-07-02T00:12:14Z

Test build #36310 has finished for PR 7070 at commit c6f3e86.

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

AmplabJenkins · 2015-07-02T00:12:53Z

Merged build finished. Test PASSed.

thegiive · 2015-07-02T03:33:49Z

HI @liancheng , thanks for the suggestion. I think your comment is really good and I have modified it already.

Please help to check if there is anything else

liancheng · 2015-07-02T16:56:04Z

@thegiive Thanks for working on this! Merging to master.

thegiive · 2015-07-02T21:24:10Z

Thanks you. @liancheng

Wisely Chen added 2 commits June 28, 2015 18:40

[SPARK-8690] add a config spark.sql.parquet.mergeSchema to disable da…

c0edb8c

…tasource API schema merge feature.

remove text

b6806fb

thegiive changed the title ~~[Spark-8690][SQL] Add a setting to disable SparkSQL parquet schema merge by using datasource API~~ [SPARK-8690][SQL] Add a setting to disable SparkSQL parquet schema merge by using datasource API Jun 28, 2015

liancheng reviewed Jun 29, 2015
View reviewed changes

Change config to SQLConf and add test case

db8ef1b

Remove some style problem

94c9307

liancheng reviewed Jul 1, 2015
View reviewed changes

Refactor some code style and merge the test case to ParquetSchemaMerg…

c6f3e86

…eConfigSuite

asfgit closed this in 246265f Jul 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-8690][SQL] Add a setting to disable SparkSQL parquet schema merge by using datasource API #7070

[SPARK-8690][SQL] Add a setting to disable SparkSQL parquet schema merge by using datasource API #7070

thegiive commented Jun 28, 2015

AmplabJenkins commented Jun 28, 2015

liancheng commented Jun 29, 2015

AmplabJenkins commented Jun 29, 2015

AmplabJenkins commented Jun 29, 2015

SparkQA commented Jun 29, 2015

SparkQA commented Jun 29, 2015

AmplabJenkins commented Jun 29, 2015

liancheng Jun 29, 2015

liancheng commented Jun 29, 2015

thegiive commented Jun 30, 2015

AmplabJenkins commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

SparkQA commented Jul 1, 2015

SparkQA commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

thegiive commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

SparkQA commented Jul 1, 2015

SparkQA commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

liancheng Jul 1, 2015

liancheng Jul 1, 2015

liancheng commented Jul 1, 2015

liancheng Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

SparkQA commented Jul 1, 2015

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

thegiive commented Jul 2, 2015

liancheng commented Jul 2, 2015

thegiive commented Jul 2, 2015



		class ParquetSchemaMergeConfigSuite extends QueryTest with ParquetTest with BeforeAndAfterAll {
		val sqlContext = TestSQLContext

[SPARK-8690][SQL] Add a setting to disable SparkSQL parquet schema merge by using datasource API #7070

[SPARK-8690][SQL] Add a setting to disable SparkSQL parquet schema merge by using datasource API #7070

Conversation

thegiive commented Jun 28, 2015

AmplabJenkins commented Jun 28, 2015

liancheng commented Jun 29, 2015

AmplabJenkins commented Jun 29, 2015

AmplabJenkins commented Jun 29, 2015

SparkQA commented Jun 29, 2015

SparkQA commented Jun 29, 2015

AmplabJenkins commented Jun 29, 2015

liancheng Jun 29, 2015

Choose a reason for hiding this comment

liancheng commented Jun 29, 2015

thegiive commented Jun 30, 2015

AmplabJenkins commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

SparkQA commented Jul 1, 2015

SparkQA commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

thegiive commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

SparkQA commented Jul 1, 2015

SparkQA commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

liancheng Jul 1, 2015

Choose a reason for hiding this comment

liancheng Jul 1, 2015

Choose a reason for hiding this comment

liancheng commented Jul 1, 2015

liancheng Jul 1, 2015

Choose a reason for hiding this comment

AmplabJenkins commented Jul 1, 2015

AmplabJenkins commented Jul 1, 2015

SparkQA commented Jul 1, 2015

SparkQA commented Jul 2, 2015

AmplabJenkins commented Jul 2, 2015

thegiive commented Jul 2, 2015

liancheng commented Jul 2, 2015

thegiive commented Jul 2, 2015