[SPARK-25036][SQL] avoid match may not be exhaustive in Scala-2.12 by kiszk · Pull Request #22014 · apache/spark

kiszk · 2018-08-06T19:27:39Z

What changes were proposed in this pull request?

The PR remove the following compilation error using scala-2.12 with sbt by adding a default case to match.

/home/ishizaki/Spark/PR/scala212/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/ValueInterval.scala:63: match may not be exhaustive.
[error] It would fail on the following inputs: (NumericValueInterval(_, _), _), (_, NumericValueInterval(_, _)), (_, _)
[error] [warn]   def isIntersected(r1: ValueInterval, r2: ValueInterval): Boolean = (r1, r2) match {
[error] [warn] 
[error] [warn] /home/ishizaki/Spark/PR/scala212/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/ValueInterval.scala:79: match may not be exhaustive.
[error] It would fail on the following inputs: (NumericValueInterval(_, _), _), (_, NumericValueInterval(_, _)), (_, _)
[error] [warn]     (r1, r2) match {
[error] [warn] 
[error] [warn] /home/ishizaki/Spark/PR/scala212/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxCountDistinctForIntervals.scala:67: match may not be exhaustive.
[error] It would fail on the following inputs: (ArrayType(_, _), _), (_, ArrayData()), (_, _)
[error] [warn]     (endpointsExpression.dataType, endpointsExpression.eval()) match {
[error] [warn] 
[error] [warn] /home/ishizaki/Spark/PR/scala212/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala:470: match may not be exhaustive.
[error] It would fail on the following inputs: NewFunctionSpec(_, None, Some(_)), NewFunctionSpec(_, Some(_), None)
[error] [warn]     newFunction match {
[error] [warn] 
[error] [warn] [error] [warn] /home/ishizaki/Spark/PR/scala212/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala:709: match may not be exhaustive.
[error] It would fail on the following input: Schema((x: org.apache.spark.sql.types.DataType forSome x not in org.apache.spark.sql.types.StructType), _)
[error] [warn]   def attributesFor[T: TypeTag]: Seq[Attribute] = schemaFor[T] match {
[error] [warn]

How was this patch tested?

Existing UTs with Scala-2.11.
Manually build with Scala-2.12

SparkQA · 2018-08-06T23:23:11Z

Test build #94314 has finished for PR 22014 at commit 9cc0c60.

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

kiszk · 2018-08-07T00:13:04Z

cc @srowen @ueshin

srowen

Looks fine. I avoid RuntimeException as it's non-specific, and prefer IllegalStateException or IllegalArgumentException in most cases. However this isn't done consistently in the code, so don't feel strongly about it at all.

HyukjinKwon · 2018-08-07T03:32:14Z

...c/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/ValueInterval.scala

        val newMax = if (n1.max <= n2.max) n1.max else n2.max
        (Some(EstimationUtils.fromDouble(newMin, dt)),
          Some(EstimationUtils.fromDouble(newMax, dt)))
+      case _ => throw new RuntimeException(s"Not supported pair: $r1, $r2 at intersect()")


Shall we do UnsupportedOperationException?

HyukjinKwon · 2018-08-07T03:36:28Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

      case NewFunctionSpec(functionName, None, None) => functionName
      case NewFunctionSpec(functionName, Some(_), Some(innerClassInstance)) =>
        innerClassInstance + "." + functionName
+      case _ => null // nothing to do since addNewFunctionInteral() must return one of them


Shall we throw an IllegalArgumentException?

HyukjinKwon · 2018-08-07T03:47:18Z

...la/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxCountDistinctForIntervals.scala

    (endpointsExpression.dataType, endpointsExpression.eval()) match {
      case (ArrayType(elementType, _), arrayData: ArrayData) =>
        arrayData.toObjectArray(elementType).map(_.toString.toDouble)
+      case _ => throw new RuntimeException("not found at endpoints")


Can we do this like:

val endpointsType = endpointsExpression.dataType.asInstanceOf[ArrayType] val endpoints = endpointsExpression.eval().asInstanceOf[ArrayData] endpoints.toObjectArray(endpointsType.elementType).map(_.toString.toDouble)

HyukjinKwon · 2018-08-07T03:51:21Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

  def attributesFor[T: TypeTag]: Seq[Attribute] = schemaFor[T] match {
    case Schema(s: StructType, _) =>
      s.toAttributes
+    case _ => throw new RuntimeException(s"$schemaFor is not supported at attributesFor()")


How about this:

case other => throw new UnsupportedOperationException(s"Attributes for type $other is not supported")

HyukjinKwon · 2018-08-07T03:51:51Z

LGTM except those rather nits.

SparkQA · 2018-08-07T19:50:12Z

Test build #94388 has finished for PR 22014 at commit 3cfbcfc.

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

dongjoon-hyun · 2018-08-07T20:16:54Z

Retest this please.

srowen · 2018-08-07T22:33:44Z

.../main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala

      // For top level row writer, it always writes to the beginning of the global buffer holder,
      // which means its fixed-size region always in the same position, so we don't need to call
      // `reset` to set up its fixed-size region every time.
-      if (inputs.map(_.isNull).forall(_ == "false")) {


@kiszk was this intentional?

Sorry, I made a mistake...

SparkQA · 2018-08-08T00:15:19Z

Test build #94390 has finished for PR 22014 at commit 3cfbcfc.

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

SparkQA · 2018-08-08T06:19:10Z

Test build #94402 has finished for PR 22014 at commit b9c11d5.

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

HyukjinKwon · 2018-08-08T06:45:56Z

Merged to master

…ala-2.12. ## What changes were proposed in this pull request? This is a follow-up pr of #22014. We still have some more compilation errors in scala-2.12 with sbt: ``` [error] [warn] /.../sql/core/src/main/scala/org/apache/spark/sql/DataFrameNaFunctions.scala:493: match may not be exhaustive. [error] It would fail on the following input: (_, _) [error] [warn] val typeMatches = (targetType, f.dataType) match { [error] [warn] [error] [warn] /.../sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/MicroBatchExecution.scala:393: match may not be exhaustive. [error] It would fail on the following input: (_, _) [error] [warn] prevBatchOff.get.toStreamProgress(sources).foreach { [error] [warn] [error] [warn] /.../sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/AggUtils.scala:173: match may not be exhaustive. [error] It would fail on the following input: AggregateExpression(_, _, false, _) [error] [warn] val rewrittenDistinctFunctions = functionsWithDistinct.map { [error] [warn] [error] [warn] /.../sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/SymmetricHashJoinStateManager.scala:271: match may not be exhaustive. [error] It would fail on the following input: (_, _) [error] [warn] keyWithIndexToValueMetrics.customMetrics.map { [error] [warn] [error] [warn] /.../sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala:959: match may not be exhaustive. [error] It would fail on the following input: CatalogTableType(_) [error] [warn] val tableTypeString = metadata.tableType match { [error] [warn] [error] [warn] /.../sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:923: match may not be exhaustive. [error] It would fail on the following input: CatalogTableType(_) [error] [warn] hiveTable.setTableType(table.tableType match { [error] [warn] ``` ## How was this patch tested? Manually build with Scala-2.12. Closes #22039 from ueshin/issues/SPARK-25036/fix_match. Authored-by: Takuya UESHIN <ueshin@databricks.com> Signed-off-by: Sean Owen <srowen@gmail.com>

…ala-2.12. ## What changes were proposed in this pull request? This is a follow-up pr of #22014 and #22039 We still have some more compilation errors in mllib with scala-2.12 with sbt: ``` [error] [warn] /home/ishizaki/Spark/PR/scala212/spark/mllib/src/main/scala/org/apache/spark/ml/evaluation/ClusteringEvaluator.scala:116: match may not be exhaustive. [error] It would fail on the following inputs: ("silhouette", _), (_, "cosine"), (_, "squaredEuclidean"), (_, String()), (_, _) [error] [warn] ($(metricName), $(distanceMeasure)) match { [error] [warn] ``` ## How was this patch tested? Existing UTs Closes #22058 from kiszk/SPARK-25036c. Authored-by: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Signed-off-by: Sean Owen <srowen@gmail.com>

…with an internal error ### What changes were proposed in this pull request? In this PR I propose to replace the legacy error class `_LEGACY_ERROR_TEMP_2014`, added as an `IllegalArgumentException` to avoid non-exhaustive case-matching in #22014, with an internal error as it is not triggered by the user space. ### Why are the changes needed? As the error is not triggered by the user space, the legacy error class can be replaced by an internal error. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No new tests were added and no tests needed changing because of the nature of the updated error class. Closes #40951 from amousavigourabi/add-new-function-internal-error. Lead-authored-by: amousavigourabi <28668597+amousavigourabi@users.noreply.github.com> Co-authored-by: Atour <28668597+amousavigourabi@users.noreply.github.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…with an internal error ### What changes were proposed in this pull request? In this PR I propose to replace the legacy error class `_LEGACY_ERROR_TEMP_2014`, added as an `IllegalArgumentException` to avoid non-exhaustive case-matching in apache#22014, with an internal error as it is not triggered by the user space. ### Why are the changes needed? As the error is not triggered by the user space, the legacy error class can be replaced by an internal error. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No new tests were added and no tests needed changing because of the nature of the updated error class. Closes apache#40951 from amousavigourabi/add-new-function-internal-error. Lead-authored-by: amousavigourabi <28668597+amousavigourabi@users.noreply.github.com> Co-authored-by: Atour <28668597+amousavigourabi@users.noreply.github.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

add default case to match

9cc0c60

srowen approved these changes Aug 7, 2018

View reviewed changes

HyukjinKwon reviewed Aug 7, 2018

View reviewed changes

address review comments

3cfbcfc

srowen reviewed Aug 7, 2018

View reviewed changes

revert an unexpected change

b9c11d5

asfgit closed this in 960af63 Aug 8, 2018

ueshin mentioned this pull request Aug 8, 2018

[SPARK-25036][SQL][FOLLOW-UP] Avoid match may not be exhaustive in Scala-2.12. #22039

Closed

kiszk mentioned this pull request Aug 9, 2018

[SPARK-25036][SQL][FOLLOW-UP] Avoid match may not be exhaustive in Scala-2.12. #22058

Closed

MaxGekk mentioned this pull request May 1, 2023

[SPARK-43250][SQL] Replace the error class _LEGACY_ERROR_TEMP_2014 with an internal error #40951

Closed

Conversation

kiszk commented Aug 6, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 6, 2018

Uh oh!

kiszk commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Aug 7, 2018

Uh oh!

SparkQA commented Aug 7, 2018

Uh oh!

dongjoon-hyun commented Aug 7, 2018

Uh oh!

srowen Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

kiszk Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 8, 2018

Uh oh!

SparkQA commented Aug 8, 2018

Uh oh!

HyukjinKwon commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kiszk commented Aug 7, 2018 •

edited

Loading