Skip to content

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

Closed
kiszk wants to merge 3 commits intoapache:masterfrom
kiszk:SPARK-25036c
Closed

[SPARK-25036][SQL][FOLLOW-UP] Avoid match may not be exhaustive in Scala-2.12.#22058
kiszk wants to merge 3 commits intoapache:masterfrom
kiszk:SPARK-25036c

Conversation

@kiszk
Copy link
Copy Markdown
Member

@kiszk kiszk commented Aug 9, 2018

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

@kiszk
Copy link
Copy Markdown
Member Author

kiszk commented Aug 9, 2018

cc @ueshin @srowen @HyukjinKwon

@srowen
Copy link
Copy Markdown
Member

srowen commented Aug 9, 2018

I see, did you run sbt one more time after this to see if there were more? would be good to address all of the ones worth changing in this pass.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 9, 2018

Test build #94509 has finished for PR 22058 at commit 5655d83.

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

case ("silhouette", "cosine") =>
CosineSilhouette.computeSilhouetteScore(df, $(predictionCol), $(featuresCol))
case (mn, dm) =>
throw new IllegalArgumentException(s"($mn, $dm) is not matched in evaluate")
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 is OK, but doesn't really add much beyond what the MatchError would have said. Worth a message like "No support for metric $mn, distance $dm"?

@kiszk
Copy link
Copy Markdown
Member Author

kiszk commented Aug 9, 2018

I think that this is the last one with the following command. With a pair of this PR and #22059, I succeeded to run sbt. But, I would like to confirm this with @ueshin.

build/sbt -Pscala-2.12 -Phadoop-2.6 -Pkubernetes -Phive -Pmesos -Phive-thriftserver -Pflume -Pkinesis-asl -Pyarn package

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 9, 2018

Test build #94517 has finished for PR 22058 at commit 3084b55.

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

@srowen
Copy link
Copy Markdown
Member

srowen commented Aug 10, 2018

Merged to master

@asfgit asfgit closed this in 1dd0f17 Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants