[SPARK-30509][SQL] Fix deprecation log warning in Avro schema inferring by MaxGekk · Pull Request #27200 · apache/spark

MaxGekk · 2020-01-14T10:24:06Z

What changes were proposed in this pull request?

In the PR, I propose to check the ignoreExtensionKey option in the case insensitive map of AvroOption.

Why are the changes needed?

The map options passed to AvroUtils.inferSchema contains all keys in the lower cases in fact. Actually, the map is converted from a CaseInsensitiveStringMap. Consequently, the check

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

Line 45 in 3663dbe

if (options.contains("ignoreExtension")) {

always return false, and the deprecation log warning is never printed.

Does this PR introduce any user-facing change?

Yes, after the changes the log warning is printed once.

How was this patch tested?

Added new test to AvroSuite which checks existence of log warning.

MaxGekk · 2020-01-14T10:24:29Z

@gengliangwang @HyukjinKwon Please, review the PR.

SparkQA · 2020-01-14T10:27:11Z

Test build #116701 has started for PR 27200 at commit 0bb33aa.

HyukjinKwon · 2020-01-14T10:40:56Z

+ .read
+ .format("avro")
+ .option(AvroOptions.ignoreExtensionKey, false)
+ .option("header", true)


Does Avro has header option?

I copy pasted the piece of code from another test in AvroSuite:
spark/external/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala
Line 1524 in 94fc0e3
.option("header", true)
. Let me check and remove it if is not needed.

Yeh, it is not needed. It seems the test which I copy-pasted was copy-pasted from another places. I guess from
spark/sql/core/src/test/scala/org/apache/spark/sql/FileBasedDataSourceSuite.scala
Lines 746 to 787 in b389b8c
test("File source v2: support partition pruning") {
withSQLConf(SQLConf.USE_V1_SOURCE_LIST.key ->"") {
allFileBasedDataSources.foreach { format =>
withTempPath { dir =>
Seq(("a", 1, 2), ("b", 1, 2), ("c", 2, 1))
.toDF("value", "p1", "p2")
.write
.format(format)
.partitionBy("p1", "p2")
.option("header", true)
.save(dir.getCanonicalPath)
valdf= spark
.read
.format(format)
.option("header", true)
.load(dir.getCanonicalPath)
.where("p1 = 1 and p2 = 2 and value != \"a\"")
valfilterCondition= df.queryExecution.optimizedPlan.collectFirst {
casef: Filter=> f.condition
}
assert(filterCondition.isDefined)
// The partitions filters should be pushed down and no need to be reevaluated.
assert(filterCondition.get.collectFirst {
casea: AttributeReferenceif a.name =="p1"|| a.name =="p2"=> a
}.isEmpty)
valfileScan= df.queryExecution.executedPlan collectFirst {
caseBatchScanExec(_, f: FileScan) => f
}
assert(fileScan.nonEmpty)
assert(fileScan.get.partitionFilters.nonEmpty)
assert(fileScan.get.planInputPartitions().forall { partition =>
partition.asInstanceOf[FilePartition].files.forall { file =>
file.filePath.contains("p1=1") && file.filePath.contains("p2=2")
}
})
checkAnswer(df, Row("b", 1, 2))
}
}
}
}
Let me remove the option from other avro test as well.

HyukjinKwon · 2020-01-14T10:41:58Z

+ val deprecatedEvents = logAppender.loggingEvents
+ .filter(_.getRenderedMessage.contains(
+ s"Option ${AvroOptions.ignoreExtensionKey} is deprecated"))
+ assert(deprecatedEvents.size === 1)


Can we test if the size is just bigger then 0 just in case we have other deprecation logs later?

I compared the size to 1 to avoid any concerns that it is printed multiple times like in the PR (#27174 (comment)), per each partition.

If we expect it is printed only once, maybe we should assert that?

So, if somebody modifies the code in the future in the way the warning is printed many times, we will catch the situation by the test.

just in case we have other deprecation logs later?
As we discussed in another PR, we are not going to print any log warnings about ignoreExtension. Am I right or misunderstood something?

SparkQA · 2020-01-14T11:36:40Z

Test build #116702 has finished for PR 27200 at commit 6c53b50.

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

dongjoon-hyun

The following will match only AvroOptions.ignoreExtensionKey. So, the warnings from another deprecations will be skipped. And, the assertion seems to verify that target warning occurs once.

valdeprecatedEvents= logAppender.loggingEvents
.filter(_.getRenderedMessage.contains(...)

dongjoon-hyun · 2020-01-14T18:03:27Z

 .write
 .format("avro")
 .partitionBy("p1", "p2")
- .option("header", true)


Ur, BTW, why do you piggy-back this removal into this PR?

The change is small, since I am here I remove unneeded option. Do you want to see a separate PR for the little change?

dongjoon-hyun

The misleading piggy-back removal is a blocker for this PR.

This reverts commit 6c53b50.

MaxGekk · 2020-01-14T18:56:05Z

@dongjoon-hyun I removed unrelated changes from this PR

SparkQA · 2020-01-14T19:40:55Z

Test build #116723 has finished for PR 27200 at commit 164cf5b.

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

dongjoon-hyun

+1, LGTM back. Merged to master.

MaxGekk added 2 commits January 14, 2020 12:36

Add a test
47879d9

Bug fix
0bb33aa

HyukjinKwon reviewed Jan 14, 2020
View reviewed changes

HyukjinKwon approved these changes Jan 14, 2020
View reviewed changes

MaxGekk added 2 commits January 14, 2020 13:55

Remove .option("header", true) in the added test
11cbf2e

Remove .option("header", true) in another test
6c53b50

dongjoon-hyun approved these changes Jan 14, 2020
View reviewed changes

dongjoon-hyun reviewed Jan 14, 2020
View reviewed changes

dongjoon-hyun added the SQL label Jan 14, 2020

dongjoon-hyun requested changes Jan 14, 2020
View reviewed changes

Revert "Remove .option("header", true) in another test"
164cf5b
This reverts commit 6c53b50.

MaxGekk mentioned this pull request Jan 14, 2020
[MINOR][TESTS] Remove unsupported header option in AvroSuite #27203
Closed

dongjoon-hyun approved these changes Jan 14, 2020
View reviewed changes

dongjoon-hyun closed this in db7262aJan 14, 2020

MaxGekk deleted the avro-fix-ignoreExtension-contains branch June 5, 2020 19:42

	test("File source v2: support partition pruning") {
	withSQLConf(SQLConf.USE_V1_SOURCE_LIST.key ->"") {
	allFileBasedDataSources.foreach { format =>
	withTempPath { dir =>
	Seq(("a", 1, 2), ("b", 1, 2), ("c", 2, 1))
	.toDF("value", "p1", "p2")
	.write
	.format(format)
	.partitionBy("p1", "p2")
	.option("header", true)
	.save(dir.getCanonicalPath)
	valdf= spark
	.read
	.format(format)
	.option("header", true)
	.load(dir.getCanonicalPath)
	.where("p1 = 1 and p2 = 2 and value != \"a\"")

	valfilterCondition= df.queryExecution.optimizedPlan.collectFirst {
	casef: Filter=> f.condition
	}
	assert(filterCondition.isDefined)
	// The partitions filters should be pushed down and no need to be reevaluated.
	assert(filterCondition.get.collectFirst {
	casea: AttributeReferenceif a.name =="p1"\|\| a.name =="p2"=> a
	}.isEmpty)

	valfileScan= df.queryExecution.executedPlan collectFirst {
	caseBatchScanExec(_, f: FileScan) => f
	}
	assert(fileScan.nonEmpty)
	assert(fileScan.get.partitionFilters.nonEmpty)
	assert(fileScan.get.planInputPartitions().forall { partition =>
	partition.asInstanceOf[FilePartition].files.forall { file =>
	file.filePath.contains("p1=1") && file.filePath.contains("p2=2")
	}
	})
	checkAnswer(df, Row("b", 1, 2))
	}
	}
	}
	}

Uh oh!

Conversation

MaxGekk commented Jan 14, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

MaxGekk commented Jan 14, 2020

Uh oh!

SparkQA commented Jan 14, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 14, 2020

Uh oh!

dongjoon-hyun left a comment • 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!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jan 14, 2020

Uh oh!

SparkQA commented Jan 14, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dongjoon-hyun left a comment •
edited
Loading