[SPARK-24213][ML] Fix for Int id type for PowerIterationClustering in spark.ml #21274
Conversation
|
Test build #90389 has finished for PR 21274 at commit
|
| dataset.schema($(idCol)).dataType match { | ||
| case _: LongType => | ||
| uncastPredictions | ||
| case otherType => |
There was a problem hiding this comment.
Why not directly use
case intType: IntType: so that make a stronger restriction ?
Or do we need to support other types besides int/long ?
|
test this please |
|
Test build #90419 has finished for PR 21274 at commit
|
|
Test build #90420 has finished for PR 21274 at commit
|
| case otherType => | ||
| uncastPredictions.select(col($(idCol)).cast(otherType).alias($(idCol))) | ||
| throw new IllegalArgumentException(s"PowerIterationClustering had an unexpected error: " + | ||
| s"ID col was found to be of type $otherType, despite initial schema checks. Please " + |
There was a problem hiding this comment.
nit: ${otherType.simpleString}
|
Test build #90427 has finished for PR 21274 at commit
|
|
LGTM. ! |
| case _: LongType => | ||
| uncastPredictions | ||
| case _: IntegerType => | ||
| uncastPredictions.withColumn($(idCol), col($(idCol)).cast(LongType)) |
There was a problem hiding this comment.
Shouldn't it be
case _: IntegerType => uncastPredictions.withColumn($(idCol), col($(idCol)).cast(IntegerType))
Otherwise it is not necessary for casting. right? Because prediction already has id as Long type and dataset has id as IntegerType. So, we need to cast prediction.id to IntegerType. right?
Correct me if I am wrong.
|
This issue actually brings up a problem with the Transformer approach for PIC. Just commented more here: https://issues.apache.org/jira/browse/SPARK-15784 Thank you for pushing back! |
5 participants
What changes were proposed in this pull request?
PIC in spark.ml has tests for "id" type IntegerType, but those tests did not fully check the result. This patch:
How was this patch tested?
Existing unit tests, with fixes