[SPARK-24068] Propagating DataFrameReader's options to Text datasource on schema inferring #21182

MaxGekk · 2018-04-27T15:54:50Z

What changes were proposed in this pull request?

While reading CSV or JSON files, DataFrameReader's options are converted to Hadoop's parameters, for example there:
https://github.com/apache/spark/blob/branch-2.3/sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala#L302

but the options are not propagated to Text datasource on schema inferring, for instance:
https://github.com/apache/spark/blob/branch-2.3/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVDataSource.scala#L184-L188

The PR proposes propagation of user's options to Text datasource on scheme inferring in similar way as user's options are converted to Hadoop parameters if schema is specified.

How was this patch tested?

The changes were tested manually by using https://github.com/twitter/hadoop-lzo:

hadoop-lzo> mvn clean package
hadoop-lzo> ln -s ./target/hadoop-lzo-0.4.21-SNAPSHOT.jar ./hadoop-lzo.jar

Create 2 test files in JSON and CSV format and compress them:

$ cat test.csv
col1|col2
a|1
$ lzop test.csv
$ cat test.json
{"col1":"a","col2":1}
$ lzop test.json

Run spark-shell with hadoop-lzo:

bin/spark-shell --jars ~/hadoop-lzo/hadoop-lzo.jar

reading compressed CSV and JSON without schema:

spark.read.option("io.compression.codecs", "com.hadoop.compression.lzo.LzopCodec").option("inferSchema",true).option("header",true).option("sep","|").csv("test.csv.lzo").show()
+----+----+
|col1|col2|
+----+----+
|   a|   1|
+----+----+

spark.read.option("io.compression.codecs", "com.hadoop.compression.lzo.LzopCodec").option("multiLine", true).json("test.json.lzo").printSchema
root
 |-- col1: string (nullable = true)
 |-- col2: long (nullable = true)

…a inferring

SparkQA · 2018-04-27T18:44:43Z

Test build #89929 has finished for PR 21182 at commit 8a8ff3f.

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

MaxGekk · 2018-04-27T21:30:56Z

jenkins, retest this, please

SparkQA · 2018-04-28T00:15:51Z

Test build #89942 has finished for PR 21182 at commit 8a8ff3f.

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

SparkQA · 2018-04-28T11:38:37Z

Test build #89953 has finished for PR 21182 at commit 1fa871d.

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

SparkQA · 2018-05-02T01:20:16Z

Test build #90010 has finished for PR 21182 at commit 9f55aa8.

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

MaxGekk · 2018-05-02T11:07:20Z

jenkins, retest this, please

SparkQA · 2018-05-02T12:55:26Z

Test build #90047 has finished for PR 21182 at commit 9f55aa8.

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

gatorsmile · 2018-05-02T21:01:58Z

cc @gengliangwang

gengliangwang · 2018-05-03T10:38:23Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala

    factory.configure(JsonParser.Feature.ALLOW_UNQUOTED_CONTROL_CHARS, allowUnquotedControlChars)
  }
+
+  val textOptions = ListMap(parameters.toList: _*)


I think we can use parameters instead of constructing a new map.

The reason why I constructed new map is I got an exception like the textOptions value is not serializable on one test suite. ListMap extends Serializable and allows to eliminate such exceptions.

I see, CaseInsensitiveMap.. with Serializable so I think it was ok. I didn't try it and run tests.

I faced to the problem on FileStreamSourceSuite.read new files in partitioned table with globbing, should not read partition data. Let me check it again.

The test fails on serialization of the textOptions value because the former one refers to transient parameters. I add the @transient annotation to textOptions (it should be safe because textOptions is using on the driver only and shouldn't be serialized).

SparkQA · 2018-05-05T13:20:36Z

Test build #90247 has finished for PR 21182 at commit 49012a3.

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

viirya · 2018-05-05T14:19:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala

    factory.configure(JsonParser.Feature.ALLOW_UNQUOTED_CONTROL_CHARS, allowUnquotedControlChars)
  }
+
+  @transient val textOptions = parameters


If you want to access parameters like this, why not just remove private on it?

My motivation for creating of the separate variable for text parameters was having one place where the text parameters are forming. So, in the future if we need additional options to pass to the Text datasource, don't need to modify all places where they are forming. @viirya If you believe, it over-complicates the implementation, I will remove it.

Seems to me it's less possibly we have new options which are not passed by parameters. If you think it can be, I'm fine with this.

Shall we make this less complicated and do what @MaxGekk's said in place next time?

viirya · 2018-05-05T23:23:06Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVOptions.scala

    settings
  }
+
+  val textOptions = ListMap(parameters.toList: _*)


Follow JSONOptions to make it @transient and use parameters instead of creating a new map?

viirya · 2018-05-05T23:48:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JSONOptions.scala

    factory.configure(JsonParser.Feature.ALLOW_UNQUOTED_CONTROL_CHARS, allowUnquotedControlChars)
  }
+
+  @transient val textOptions = parameters


Seems to me it's less possibly we have new options which are not passed by parameters. If you think it can be, I'm fine with this.

HyukjinKwon · 2018-05-06T07:57:56Z

Seems fine otherwise.

HyukjinKwon · 2018-05-06T08:17:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVOptions.scala

 import java.nio.charset.StandardCharsets
 import java.util.{Locale, TimeZone}

+import scala.collection.immutable.ListMap


nit: seems not used.

SparkQA · 2018-05-06T11:32:16Z

Test build #90270 has finished for PR 21182 at commit d138c44.

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

SparkQA · 2018-05-06T13:13:03Z

Test build #90269 has finished for PR 21182 at commit cd1ffad.

This patch fails from timeout after a configured wait of `300m`.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2018-05-06T15:09:30Z

jenkins, retest this, please

SparkQA · 2018-05-06T15:39:02Z

Test build #90276 has finished for PR 21182 at commit 6610826.

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

SparkQA · 2018-05-06T16:47:21Z

Test build #90284 has finished for PR 21182 at commit 6610826.

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

MaxGekk · 2018-05-06T17:03:42Z

jenkins, retest this, please

SparkQA · 2018-05-06T20:59:51Z

Test build #90286 has finished for PR 21182 at commit 6610826.

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

MaxGekk · 2018-05-08T19:07:24Z

@HyukjinKwon @viirya @gengliangwang May I ask you to look at the PR again.

gengliangwang

LGTM

HyukjinKwon · 2018-05-09T00:32:31Z

Merged to master.

cloud-fan · 2018-05-09T05:45:39Z

shall we backport it to 2.3?

HyukjinKwon · 2018-05-09T06:05:57Z

I tried but it had conflict when I merged this. @MaxGekk mind opening a backport pr?

MaxGekk · 2018-05-09T09:30:47Z

@HyukjinKwon Sure, I will prepare a PR

MaxGekk · 2018-05-10T13:01:33Z

Here is the backport to 2.3: #21292

Propagating DataFrameReader's options to the text datasource on schem…

8a8ff3f

…a inferring

Make textOptions serializable

1fa871d

Merge branch 'master' into text-options

9f55aa8

gengliangwang reviewed May 3, 2018

View reviewed changes

Adding @transient to textOptions because they shouldn't be serialized

49012a3

viirya reviewed May 5, 2018

View reviewed changes

Removing the separate val for textOptions

cd1ffad

HyukjinKwon reviewed May 6, 2018

View reviewed changes

Removing unused imports

d138c44

Merge remote-tracking branch 'origin/master' into text-options

6610826

gengliangwang approved these changes May 8, 2018

View reviewed changes

asfgit closed this in e3de6ab May 9, 2018

MaxGekk deleted the text-options branch August 17, 2019 13:34

[SPARK-24068] Propagating DataFrameReader's options to Text datasource on schema inferring #21182

[SPARK-24068] Propagating DataFrameReader's options to Text datasource on schema inferring #21182

Uh oh!

Conversation

MaxGekk commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 27, 2018

Uh oh!

MaxGekk commented Apr 27, 2018

Uh oh!

SparkQA commented Apr 28, 2018

Uh oh!

SparkQA commented Apr 28, 2018

Uh oh!

SparkQA commented May 2, 2018

Uh oh!

MaxGekk commented May 2, 2018

Uh oh!

SparkQA commented May 2, 2018

Uh oh!

gatorsmile commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented May 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 6, 2018

Uh oh!

SparkQA commented May 6, 2018

Uh oh!

MaxGekk commented May 6, 2018

Uh oh!

SparkQA commented May 6, 2018

Uh oh!

SparkQA commented May 6, 2018

Uh oh!

MaxGekk commented May 6, 2018

Uh oh!

SparkQA commented May 6, 2018

Uh oh!

MaxGekk commented May 8, 2018

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented May 9, 2018

Uh oh!

cloud-fan commented May 9, 2018

Uh oh!

HyukjinKwon commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MaxGekk commented Apr 27, 2018 •

edited

Loading

MaxGekk May 3, 2018 •

edited

Loading

HyukjinKwon commented May 9, 2018 •

edited

Loading