[SPARK-24805][SQL] Do not ignore avro files without extensions by default #21769

MaxGekk · 2018-07-14T11:00:41Z

What changes were proposed in this pull request?

In the PR, I propose to change default behaviour of AVRO datasource which currently ignores files without .avro extension in read by default. This PR sets the default value for avro.mapred.ignore.inputs.without.extension to false in the case if the parameter is not set by an user.

How was this patch tested?

Added a test file without extension in AVRO format, and new test for reading the file with and wihout specified schema.

MaxGekk · 2018-07-14T11:01:20Z

@gengliangwang @gatorsmile Please, have a look at the PR.

SparkQA · 2018-07-14T11:18:37Z

Test build #93000 has finished for PR 21769 at commit 8562a8d.

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

gengliangwang · 2018-07-14T17:58:09Z

external/avro/src/main/scala/org/apache/spark/sql/avro/AvroFileFormat.scala

    // figure out the schema of the whole dataset.
    val sampleFile =
-      if (conf.getBoolean(AvroFileFormat.IgnoreFilesWithoutExtensionProperty, true)) {
+      if (AvroFileFormat.ignoreFilesWithoutExtensions(conf)) {


I tried running queries. The option avro.mapred.ignore.inputs.without.extension is not set in conf. This is a bug in spark-avro.
Please read the value from options. It would be good to have a new test case with avro.mapred.ignore.inputs.without.extension as true.

The avro.mapred.ignore.inputs.without.extension is hadoop's parameter. This PR aims to change the default behavior only. I would prefer to do not convert the hadoop parameter to Avro datasource option here.

Here is how people use the option so far: databricks/spark-avro#71 (comment) . Probably we should discuss seperatly from this PR how we could fix the "bug" and could not break backward compatibily.

The Hadoop config can be changed like:

spark .sqlContext .sparkContext .hadoopConfiguration .set("avro.mapred.ignore.inputs.without.extension", "true")

Can we submit a separate PR to add a new option for AVRO? We should not rely on hadoopConf to control the behaviors of AVRO.

Here is the PR: #21798 Please, have a look at it.

gengliangwang · 2018-07-14T18:01:00Z

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala

    }
  }
+
+  test("SPARK-24805: reading files without .avro extension") {


Nit: Can we create a temp path and copy the original episodes.avro to the path? So that we don't need to have two duplicated resource file.

Does it just introduce unnesseccary dependency here and overcomplicate the test? I can create small (with just one row) avro file without .avro extension especially for the test if you don't mind.

gengliangwang · 2018-07-14T18:02:39Z

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala

+    intercept[java.io.IOException] {
      TestUtils.withTempDir { dir =>
        FileUtils.touch(new File(dir, "test"))
        spark.read.avro(dir.toString)


We can fix the case as

spark.read.option("avro.mapred.ignore.inputs.without.extension", false).avro(dir.toString)

The behavior will be the same as before. And we don't need to modify the expected FileNotFoundException

I would actually remove this piece of code from the test. It checked the default behavior but it is checked by special test now. Explicit settings for avro.mapred.ignore.inputs.without.extension should be checked in separate tests where the config is set explicitly.

SparkQA · 2018-07-14T22:28:24Z

Test build #93008 has finished for PR 21769 at commit a7d078e.

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

SparkQA · 2018-07-14T23:23:30Z

Test build #93009 has finished for PR 21769 at commit 3b75c27.

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

SparkQA · 2018-07-15T10:35:22Z

Test build #93023 has finished for PR 21769 at commit bb1098f.

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

gengliangwang · 2018-07-15T12:13:29Z

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala

-        .read
-        .option(AvroFileFormat.IgnoreFilesWithoutExtensionProperty, "true")
-        .avro(tempSaveDir)
+      val count = try {


Nit: consider writing the try...finally like this:

val hadoopConf = spark.sqlContext.sparkContext.hadoopConfiguration try { hadoopConf.set(AvroFileFormat.IgnoreFilesWithoutExtensionProperty, "true") val count = spark.read.avro(tempSaveDir).count() assert(count == 8) } finally { hadoopConf.unset(AvroFileFormat.IgnoreFilesWithoutExtensionProperty) }

…folder.

SparkQA · 2018-07-15T20:59:10Z

Test build #93034 has finished for PR 21769 at commit 91e40e7.

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

SparkQA · 2018-07-16T07:13:55Z

Test build #93086 has finished for PR 21769 at commit a7f3835.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class AvroDeserializer(rootAvroType: Schema, rootCatalystType: DataType)
sealed trait CatalystDataUpdater
final class RowUpdater(row: InternalRow) extends CatalystDataUpdater
final class ArrayDataUpdater(array: ArrayData) extends CatalystDataUpdater
class AvroSerializer(rootCatalystType: DataType, rootAvroType: Schema, nullable: Boolean)
class IncompatibleSchemaException(msg: String, ex: Throwable = null) extends Exception(msg, ex)
class SerializableSchema(@transient var value: Schema)

SparkQA · 2018-07-16T09:10:53Z

Test build #93102 has finished for PR 21769 at commit 134c724.

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

SparkQA · 2018-07-16T18:55:10Z

Test build #93126 has finished for PR 21769 at commit 85cdf87.

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

MaxGekk · 2018-07-16T19:09:36Z

@gengliangwang @gatorsmile Please, have a look at the PR.

gatorsmile · 2018-07-16T21:35:19Z

LGTM since we should still keep the original behavior untouched.

Thanks! Merged to master.

BTW, can we submit a separate PR to add a new option for AVRO? We should not rely on hadoopConf to control the behaviors of AVRO in general. Let us support both.

MaxGekk · 2018-07-16T21:38:51Z

can we submit a separate PR to add a new option for AVRO?

Sure, I will do.

…ault In the PR, I propose to change default behaviour of AVRO datasource which currently ignores files without `.avro` extension in read by default. This PR sets the default value for `avro.mapred.ignore.inputs.without.extension` to `false` in the case if the parameter is not set by an user. Added a test file without extension in AVRO format, and new test for reading the file with and wihout specified schema. Author: Maxim Gekk <[email protected]> Author: Maxim Gekk <[email protected]> Closes apache#21769 from MaxGekk/avro-without-extension. (cherry picked from commit ba437fc)

MaxGekk added 3 commits July 14, 2018 11:49

Test for reading files without avro extension

35063ef

Fix tests

760f98e

Adding ticket number to test title

8562a8d

MaxGekk changed the title ~~[SPARK-24805][SQL] Do not ignore avro files without extensions~~ [SPARK-24805][SQL] Do not ignore avro files without extensions by default Jul 14, 2018

gengliangwang reviewed Jul 14, 2018

View reviewed changes

gengliangwang mentioned this pull request Jul 14, 2018

Do not ignore avro files without extensions by default databricks/spark-avro#286

Merged

Revert expected exception in the test

a7d078e

Fix tests

3b75c27

Merge remote-tracking branch 'origin/master' into avro-without-extension

bb1098f

gengliangwang reviewed Jul 15, 2018

View reviewed changes

MaxGekk added 2 commits July 15, 2018 22:19

Simplification of tests

a48770c

Copying the episodes file intead of having duplicate in the resource …

91e40e7

…folder.

Merge branch 'master' into avro-without-extension

a7f3835

MaxGekk added 2 commits July 16, 2018 10:51

Fit test

1c53251

Merge remote-tracking branch 'origin/master' into avro-without-extension

134c724

Fix test - using URI

85cdf87

asfgit closed this in ba437fc Jul 16, 2018

MaxGekk deleted the avro-without-extension branch August 17, 2019 13:35

[SPARK-24805][SQL] Do not ignore avro files without extensions by default #21769

[SPARK-24805][SQL] Do not ignore avro files without extensions by default #21769

Uh oh!

Conversation

MaxGekk commented Jul 14, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

MaxGekk commented Jul 14, 2018

Uh oh!

SparkQA commented Jul 14, 2018

Uh oh!

gengliangwang Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jul 14, 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!

MaxGekk Jul 14, 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!

MaxGekk Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

gatorsmile Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jul 17, 2018

Choose a reason for hiding this comment

Uh oh!

gengliangwang Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jul 14, 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!

gengliangwang Jul 14, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jul 14, 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!

SparkQA commented Jul 14, 2018

Uh oh!

SparkQA commented Jul 14, 2018

Uh oh!

SparkQA commented Jul 15, 2018

Uh oh!

gengliangwang Jul 15, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 15, 2018

Uh oh!

SparkQA commented Jul 16, 2018

Uh oh!

SparkQA commented Jul 16, 2018

Uh oh!

SparkQA commented Jul 16, 2018

Uh oh!

MaxGekk commented Jul 16, 2018

Uh oh!

gatorsmile commented Jul 16, 2018

Uh oh!

MaxGekk commented Jul 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MaxGekk Jul 14, 2018 •

edited

Loading

MaxGekk Jul 14, 2018 •

edited

Loading

MaxGekk Jul 14, 2018 •

edited

Loading

MaxGekk Jul 14, 2018 •

edited

Loading