Skip to content

[SQL][MINOR] Fix compiling for scala 2.12 #22260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
da-liii wants to merge 4 commits into from
Closed

[SQL][MINOR] Fix compiling for scala 2.12 #22260

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
626f265
Fix exhaustive match using collect
Aug 29, 2018
ffdb12d
scala.util.Properties is more general
Aug 29, 2018
f30f47d
same semantics with an IllegalStateException
Aug 30, 2018
2dff17b
fix
Aug 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/ProjectionOverSchema.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private[execution] case class ProjectionOverSchema(schema: StructType) {
case GetArrayItem(child, arrayItemOrdinal) =>
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 {

@maropu maropu Aug 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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("....")
        }

@da-liii da-liii Aug 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@srowen srowen Aug 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 da-liii Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use collect, one must dive into SPARK-4502.

This PR is intended for #22264 .

I will preserve the error behavior.

case (projection, ArrayType(projSchema @ StructType(_), _)) =>
GetArrayStructFields(projection,
projSchema(a.field.name),
Expand All @@ -49,7 +49,7 @@ private[execution] case class ProjectionOverSchema(schema: StructType) {
case GetMapValue(child, key) =>
getProjection(child).map { projection => GetMapValue(projection, key) }
case GetStructFieldObject(child, field: StructField) =>
getProjection(child).map(p => (p, p.dataType)).map {
getProjection(child).map(p => (p, p.dataType)).collect {
case (projection, projSchema: StructType) =>
GetStructField(projection, projSchema.fieldIndex(field.name))
}
Expand Down
2 changes: 1 addition & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package org.apache.spark.sql.hive

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

import scala.tools.nsc.Properties
import scala.util.Properties

@srowen srowen Aug 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works in 2.11 and 2.12?

@da-liii da-liii Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. And

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

@da-liii da-liii Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scala.tools is part of scala-compiler


import org.apache.hadoop.fs.Path
import org.scalatest.{BeforeAndAfterEach, Matchers}
Expand Down