[SPARK-25036][SQL][FOLLOW-UP] Avoid match may not be exhaustive in Scala-2.12.#22039
[SPARK-25036][SQL][FOLLOW-UP] Avoid match may not be exhaustive in Scala-2.12.#22039ueshin wants to merge 2 commits intoapache:masterfrom
Conversation
| reader.commit(reader.deserializeOffset(off.json)) | ||
| case (src, _) => | ||
| throw new IllegalArgumentException( | ||
| s"Unknows source is found at constructNextBatch: $src") |
| s.copy(desc = newDesc(desc)) -> value | ||
| case (s, _) => | ||
| throw new IllegalArgumentException( | ||
| s"Unknown state store custom metric is found at metrics: $s") |
There was a problem hiding this comment.
nit: 2 more spaces for indentation?
|
Test build #94440 has finished for PR 22039 at commit
|
|
Test build #94435 has finished for PR 22039 at commit
|
|
Also merged to master |
|
Hmmm... sorry to be late to this, but making pattern matches exhaustive by adding a catch-all case that then throws an exception, while easy, should be considered as a less than optimal fix. Ideally, the type handling should be tightened up so that the match can be exhaustive without a catch-all. The reason for this is that if in the future a type is added such that the pattern match should be extended to handle that type, then the presence of a catch-all will give the false impression that the new type is being handled, no compiler warning will be thrown, etc. If the pattern match is made exhaustive without a catch-all and the compiler option to convert warnings to errors is used, then it becomes pretty much impossible that future type additions/additional necessary cases will be silently mishandled. Now I realize that it is not always feasible to achieve that level of type safety in Scala code, but has adequate consideration been given to making the effort here, or was this just a quick hack to make the compiler shut up? |
|
Yes, I agree about the future-proofing problem. However, at the moment, new future cases will also not cause any compile problem, but will just also trigger a MatchError. This is, simply, always a problem, that new future unmatched cases aren't handled and trigger an exception at runtime not compile time. These changes at least make the error more explicit. Of course, if the default case in any of these situations really had a valid outcome, then throwing an exception is wrong. But I don't see any reason to believe that, currently, a match is incorrectly handled as MatchError. I think it's OK to remove the compile-time warning by supplying a good default exception case. I think tests will help figure out where those default cases are no longer appropriate. Yes, "quick hack", but, as opposed to what in these specific cases? |
Yes, that is the key question. I'll admit, I haven't looked at all deeply to try to figure out whether something better is possible, so this is just my knee-jerk reaction: If you don't have to, then don't use a catch-all just to silence the compiler warning. If you're satisfied that there isn't a better option, then I'll accept that second-best is the best we can do here. |
…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>
6 participants
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:
How was this patch tested?
Manually build with Scala-2.12.