[SPARK-14891][ML] Add schema validation for ALS #12762

MLnick · 2016-04-28T18:58:53Z

This PR adds schema validation to ml's ALS and ALSModel. Currently, no schema validation was performed as transformSchema was never called in ALS.fit or ALSModel.transform. Furthermore, due to no schema validation, if users passed in Long (or Float etc) ids, they would be silently cast to Int with no warning or error thrown.

With this PR, ALS now supports all numeric types for user, item, and rating columns. The rating column is cast to Float and the user and item cols are cast to Int (as is the case currently) - however for user/item, the cast throws an error if the value is outside integer range. Behavior for rating col is unchanged (as it is not an issue).

How was this patch tested?

New test cases in ALSSuite.

MLnick · 2016-04-28T18:59:37Z

cc @sethah @BenFradet @srowen @jkbradley @yanboliang

holdenk · 2016-04-28T19:07:30Z

mllib/src/main/scala/org/apache/spark/ml/recommendation/ALS.scala

So it seems like the only other numeric type we support is Long, maybe it would be better to say that? Someone might try and pass in BigInts or Doubles and expect this work.

We "support" all numeric types in the sense that the input col can be any numeric type. But it is cast to Int. It is a "safe" cast though, if it is > Int.MaxValue or < Int.MinValue it throws an exception. "Safe" in the sense that it won't mangle the user's input ids (e.g. if Longs are passed in they will now get a failure on fit rather than a silent cast of those Long ids into Ints).

Ah yes, I didn't notice the first cast from input type to Long - it seems like that would be OK[ish] most of the time (except with floats/doubles), but also with certain BigDecimal you could end up throwing away the high bits when going to a Long and a very out of range value would pass the range check.

Ah ok. Could cast to double or float here... I was just concerned about any
storage / performance impact, but if everything is pipelines through the
cast -> udf then no problem
On Thu, 28 Apr 2016 at 21:27, Holden Karau notifications@github.com wrote:

In mllib/src/main/scala/org/apache/spark/ml/recommendation/ALS.scala
#12762 (comment):

/** @group getParam */
def getUserCol: String = $(userCol)

/**

* Param for the column name for item ids.

* Param for the column name for item ids. Ids must be integers. Other

* numeric types are supported for this column, but will be cast to integers as long as they

Ah yes, I didn't notice the first cast from input type to Long - it seems
like that would be OK[ish] most of the time (except with floats/doubles),
but also with certain BigDecimal you could end up throwing away the high
bits when going to a Long and a very out of range value would pass the
range check.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/12762/files/73ea0b62f1c0ae6a9897ec83f5c8dfedea86f3f9#r61487974

I think keeping cast to Integer is good for performance - but maybe just avoiding supporting input types that we might silently fail on/produce junk results for.

cc @jkbradley

Personally as per the related JIRA, I would actually like to support Int, Long and String for ids in ALS (with appropriate warnings about performance impact for Long/String ids). For the vast majority of use cases I believe the user-friendliness of supporting String in particular outweighs the performance impact. For those users who need performance at scale, they can stick to Int.

But for now, since only Int ids are supported in the DF API, some validation is better than nothing. I am actually slightly more in favor of only supporting Int or Long for the id columns in this PR, since the real-world occurrence of a Double or other more esoteric numeric type for the id column is, IMO, highly unlikely, and in that case requiring users to do the cast explicitly themselves is ok I would say.

So we can support only Int and Longs (within Integer range) as a simpler alternative here - it would just require to update the type checks in transformSchema and the tests.

@jkbradley @srowen @holdenk thoughts?

ping @mengxr also in case you have a chance to take a look, and consider this question of whether to only support Int/Long for ids or support all numeric types (with "safe" cast to Int in both cases)

Sorry for the slow answer. I like supporting all Numeric types, with checking. I agree we should support String IDs at some point, with automatic indexing; that can be part of this discussion: [https://issues.apache.org/jira/browse/SPARK-11106]

SparkQA · 2016-04-28T19:39:55Z

Test build #57267 has finished for PR 12762 at commit 73ea0b6.

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

BenFradet · 2016-04-28T20:30:39Z

mllib/src/test/scala/org/apache/spark/ml/recommendation/ALSSuite.scala

not sure this is used

ah yes that's a left-over, will remove.

BenFradet · 2016-04-28T21:11:36Z

LGTM except for a few minors.

SparkQA · 2016-04-29T07:25:14Z

Test build #57319 has finished for PR 12762 at commit fb443ef.

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

yanboliang · 2016-04-29T15:30:37Z

This looks good to me. Thanks!

MLnick · 2016-05-04T13:08:11Z

@yanboliang @holdenk @BenFradet @jkbradley I went ahead and just cast user/item col to double before checked cast to support all numeric types for user/item ids.

SparkQA · 2016-05-04T13:44:13Z

Test build #57759 has finished for PR 12762 at commit 70a29f5.

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

SparkQA · 2016-05-11T19:52:15Z

Test build #58395 has finished for PR 12762 at commit f0ecde7.

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

BenFradet · 2016-05-11T19:57:49Z

LGTM, except for maybe the generics in checkNumericTypesALS but that's really minor.

SparkQA · 2016-05-12T07:36:12Z

Test build #58455 has finished for PR 12762 at commit 150321f.

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

SparkQA · 2016-05-16T09:51:43Z

Test build #58633 has finished for PR 12762 at commit 1fd1f87.

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

MLnick · 2016-05-18T12:14:04Z

jenkins retest this please

SparkQA · 2016-05-18T12:55:49Z

Test build #58776 has finished for PR 12762 at commit 1fd1f87.

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

This PR adds schema validation to `ml`'s ALS and ALSModel. Currently, no schema validation was performed as `transformSchema` was never called in `ALS.fit` or `ALSModel.transform`. Furthermore, due to no schema validation, if users passed in Long (or Float etc) ids, they would be silently cast to Int with no warning or error thrown. With this PR, ALS now supports all numeric types for `user`, `item`, and `rating` columns. The rating column is cast to `Float` and the user and item cols are cast to `Int` (as is the case currently) - however for user/item, the cast throws an error if the value is outside integer range. Behavior for rating col is unchanged (as it is not an issue). ## How was this patch tested? New test cases in `ALSSuite`. Author: Nick Pentreath <nickp@za.ibm.com> Closes #12762 from MLnick/SPARK-14891-als-validate-schema. (cherry picked from commit e8b79af) Signed-off-by: Nick Pentreath <nickp@za.ibm.com>

MLnick · 2016-05-18T19:14:02Z

Merged to master/branch-2.0

jkbradley · 2016-05-27T01:53:18Z

mllib/src/main/scala/org/apache/spark/ml/recommendation/ALS.scala

-      .join(userFactors, dataset($(userCol)) === userFactors("id"), "left")
-      .join(itemFactors, dataset($(itemCol)) === itemFactors("id"), "left")
+      .join(userFactors,
+        checkedCast(dataset($(userCol)).cast(DoubleType)) === userFactors("id"), "left")


Shouldn't this and the next line's cast both use IntegerType?

@jkbradley the existing code did the cast to Int - that means passing in Long or Double (say) would silently cast and potentially lose precison and give weird results, with no exception or warning. That's why here we cast to DoubleType and use the checkedCast udf to do a safe cast to Int if the value is within Int value range. If not we throw an exception with a helpful message.

This is so we can allow any numeric type for the user/item columns (providing some form of "backward compatability" with the old version that didn't check types at all), but we can still only support actual values that are Ints.

Right, makes sense, thanks!

holdenk reviewed Apr 28, 2016
View reviewed changes

BenFradet reviewed Apr 28, 2016
View reviewed changes

MLnick force-pushed the SPARK-14891-als-validate-schema branch from fb443ef to 2a7bb9f Compare May 4, 2016 13:02

MLnick force-pushed the SPARK-14891-als-validate-schema branch from 70a29f5 to f0ecde7 Compare May 11, 2016 19:10

Nick Pentreath added 11 commits May 16, 2016 10:36

Intial work

c794317

Improve tests and add transform test cases + small cleanup

878baf5

revert small erroneous addition

a6965ce

user/item col param docs

3e84f4c

indentation and unused val

48e7b6b

user/item col cast to DoubleType before checkedCast

1b49801

Fix erroneous indentation change

d663a16

update to SparkSession

676daf9

Get rid of unnecessary generic test method for ALS

34a0700

test using float

b0aa98f

Update user/item col doc (also in PySpark)

1fd1f87

MLnick force-pushed the SPARK-14891-als-validate-schema branch from 150321f to 1fd1f87 Compare May 16, 2016 09:10

asfgit closed this in e8b79af May 18, 2016

jkbradley reviewed May 27, 2016
View reviewed changes

MLnick deleted the SPARK-14891-als-validate-schema branch May 27, 2016 07:52

MLnick mentioned this pull request Mar 1, 2017

[SPARK-19733][ML]Removed unnecessary castings and refactored checked casts in ALS. #17059

Closed

[SPARK-14891][ML] Add schema validation for ALS #12762

[SPARK-14891][ML] Add schema validation for ALS #12762

Uh oh!

Conversation

MLnick commented Apr 28, 2016

How was this patch tested?

Uh oh!

MLnick commented Apr 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MLnick Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MLnick Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenFradet commented Apr 28, 2016

Uh oh!

SparkQA commented Apr 29, 2016

Uh oh!

yanboliang commented Apr 29, 2016

Uh oh!

MLnick commented May 4, 2016

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

SparkQA commented May 11, 2016

Uh oh!

BenFradet commented May 11, 2016

Uh oh!

SparkQA commented May 12, 2016

Uh oh!

SparkQA commented May 16, 2016

Uh oh!

MLnick commented May 18, 2016

Uh oh!

SparkQA commented May 18, 2016

Uh oh!

MLnick commented May 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MLnick May 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MLnick Apr 28, 2016 •

edited

Loading

MLnick Apr 29, 2016 •

edited

Loading

MLnick May 27, 2016 •

edited

Loading