[SQL][MINOR] Fix compiling for scala 2.12 by da-liii · Pull Request #22260 · apache/spark

da-liii · 2018-08-29T03:22:27Z

What changes were proposed in this pull request?

Introduced by #21320 and #11744

$ sbt
> ++2.12.6
> project sql
> compile
...
[error] [warn] spark/sql/core/src/main/scala/org/apache/spark/sql/execution/ProjectionOverSchema.scala:41: match may not be exhaustive.
[error] It would fail on the following inputs: (_, ArrayType(_, _)), (_, _)
[error] [warn] getProjection(a.child).map(p => (p, p.dataType)).map {
[error] [warn]
[error] [warn] spark/sql/core/src/main/scala/org/apache/spark/sql/execution/ProjectionOverSchema.scala:52: match may not be exhaustive.
[error] It would fail on the following input: (_, _)
[error] [warn] getProjection(child).map(p => (p, p.dataType)).map {
[error] [warn]
...

And

$ sbt
> ++2.12.6
> project hive
> testOnly *ParquetMetastoreSuite
...
[error] /Users/rendong/wdi/spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:22: object tools is not a member of package scala
[error] import scala.tools.nsc.Properties
[error] ^
[error] /Users/rendong/wdi/spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:146: not found: value Properties
[error] val version = Properties.versionNumberString match {
[error] ^
[error] two errors found
...

How was this patch tested?

Existing tests.

maropu · 2018-08-29T03:42:19Z

 getProjection(child).map { projection => GetArrayItem(projection, arrayItemOrdinal) }
 case a: GetArrayStructFields =>
- getProjection(a.child).map(p => (p, p.dataType)).map {
+ getProjection(a.child).map(p => (p, p.dataType)).collect {


How about this? IMO .collect can't catch illegal inputs?
getProjection(a.child).map(p => (p, p.dataType)).map { case (projection, ArrayType(projSchema @ StructType(_), _)) => GetArrayStructFields(projection, projSchema(a.field.name), projSchema.fieldIndex(a.field.name), projSchema.size, a.containsNull) case _ => sys.error("....") }

Well, there is a semantic change using collect. I will provide a unit test to verify the change or simply adopt sys.error

Does this generate an error or just a warning in 2.12? It's an unmatched case error, right?
We had 'fixed' this elsewhere with case _ => throw new IllegalStateException(...) or something. At runtime it would have caused a MatchError before, right? Because it didn't, I hope, or else tests would have failed, it matters a bit less what happens in this case. But it felt more conservative to preserve the error behavior; maybe I was wrong about that?

To use collect, one must dive into SPARK-4502.
This PR is intended for #22264 .
I will preserve the error behavior.

maropu · 2018-08-29T03:43:56Z

Can you add [SQL][MINOR] in the title? Also, can you narrow down the tittle cuz it is a little obscure.

HyukjinKwon · 2018-08-29T04:03:26Z

ok to test

SparkQA · 2018-08-29T07:05:02Z

Test build #95401 has finished for PR 22260 at commit 626f265.

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

SparkQA · 2018-08-29T09:26:32Z

Test build #95406 has finished for PR 22260 at commit ffdb12d.

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

srowen · 2018-08-29T17:21:31Z

 import java.io.{BufferedWriter, File, FileWriter}

-import scala.tools.nsc.Properties
+import scala.util.Properties


This works in 2.11 and 2.12?

Yes. And
scala.util.Properties is better than scala.tools.nsc.Properties !

scala.tools is part of scala-compiler

srowen · 2018-08-29T17:29:49Z

 getProjection(child).map { projection => GetArrayItem(projection, arrayItemOrdinal) }
 case a: GetArrayStructFields =>
- getProjection(a.child).map(p => (p, p.dataType)).map {
+ getProjection(a.child).map(p => (p, p.dataType)).collect {


Does this generate an error or just a warning in 2.12? It's an unmatched case error, right?
We had 'fixed' this elsewhere with case _ => throw new IllegalStateException(...) or something. At runtime it would have caused a MatchError before, right? Because it didn't, I hope, or else tests would have failed, it matters a bit less what happens in this case. But it felt more conservative to preserve the error behavior; maybe I was wrong about that?

da-liii · 2018-08-30T02:22:58Z

@maropu @srowen please review

maropu · 2018-08-30T02:29:53Z

LGTM, too.

SparkQA · 2018-08-30T06:12:22Z

Test build #95452 has finished for PR 22260 at commit 2dff17b.

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

HyukjinKwon · 2018-08-30T07:04:55Z

Merged to master.

## What changes were proposed in this pull request? Introduced by apache#21320 and apache#11744 ``` $ sbt > ++2.12.6 > project sql > compile ... [error] [warn] spark/sql/core/src/main/scala/org/apache/spark/sql/execution/ProjectionOverSchema.scala:41: match may not be exhaustive. [error] It would fail on the following inputs: (_, ArrayType(_, _)), (_, _) [error] [warn] getProjection(a.child).map(p => (p, p.dataType)).map { [error] [warn] [error] [warn] spark/sql/core/src/main/scala/org/apache/spark/sql/execution/ProjectionOverSchema.scala:52: match may not be exhaustive. [error] It would fail on the following input: (_, _) [error] [warn] getProjection(child).map(p => (p, p.dataType)).map { [error] [warn] ... ``` And ``` $ sbt > ++2.12.6 > project hive > testOnly *ParquetMetastoreSuite ... [error] /Users/rendong/wdi/spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:22: object tools is not a member of package scala [error] import scala.tools.nsc.Properties [error] ^ [error] /Users/rendong/wdi/spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:146: not found: value Properties [error] val version = Properties.versionNumberString match { [error] ^ [error] two errors found ... ``` ## How was this patch tested? Existing tests. Closesapache#22260 from sadhen/fix_exhaustive_match. Authored-by: 忍冬 <rendong@wacai.com> Signed-off-by: hyukjinkwon <gurwls223@apache.org>

Fix exhaustive match using collect
626f265

maropu reviewed Aug 29, 2018
View reviewed changes

da-liii changed the title ~~[MINOR] Fix scala 2.12 build using collect~~[SQL][MINOR] Fix compiling for scala 2.12Aug 29, 2018

da-liii changed the title ~~[SQL][MINOR] Fix compiling for scala 2.12~~[WIP][SQL][MINOR] Fix compiling for scala 2.12Aug 29, 2018

scala.util.Properties is more general
ffdb12d

da-liii changed the title ~~[WIP][SQL][MINOR] Fix compiling for scala 2.12~~[SQL][MINOR] Fix compiling for scala 2.12Aug 29, 2018

da-liii mentioned this pull request Aug 29, 2018
[SPARK-25256][SQL][TEST] Plan mismatch errors in Hive tests in Scala 2.12 #22264
Closed

srowen reviewed Aug 29, 2018
View reviewed changes

忍冬 added 2 commits August 30, 2018 10:19

same semantics with an IllegalStateException
f30f47d

fix
2dff17b

srowen approved these changes Aug 30, 2018
View reviewed changes

asfgit closed this in 56bc700Aug 30, 2018

Uh oh!

Conversation

da-liii commented Aug 29, 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!

maropuAug 29, 2018

Choose a reason for hiding this comment

Uh oh!

da-liiiAug 29, 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!

srowenAug 29, 2018

Choose a reason for hiding this comment

Uh oh!

da-liiiAug 30, 2018

Choose a reason for hiding this comment

Uh oh!

maropu commented Aug 29, 2018

Uh oh!

HyukjinKwon commented Aug 29, 2018

Uh oh!

SparkQA commented Aug 29, 2018

Uh oh!

SparkQA commented Aug 29, 2018

Uh oh!

srowenAug 29, 2018

Choose a reason for hiding this comment

Uh oh!

da-liiiAug 30, 2018

Choose a reason for hiding this comment

Uh oh!

da-liiiAug 30, 2018

Choose a reason for hiding this comment

Uh oh!

srowenAug 29, 2018

Choose a reason for hiding this comment

Uh oh!

da-liii commented Aug 30, 2018

Uh oh!

maropu commented Aug 30, 2018

Uh oh!

SparkQA commented Aug 30, 2018

Uh oh!

HyukjinKwon commented Aug 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

da-liii commented Aug 29, 2018•
edited
Loading

da-liiiAug 29, 2018•
edited
Loading