[SPARK-10835] [ML] Word2Vec should accept non-null string array, in addition to existing null string array

[srowen]

What changes were proposed in this pull request?

To match Tokenizer and for compatibility with Word2Vec, output a nullable string array type in NGram

How was this patch tested?

Jenkins tests.

[srowen]

This change merely parallels #7414 from @hhbyyh (what do you think?) I'd still sort of like to understand why we didn't 'fix' Word2Vec instead in that change?

[SparkQA]

Test build #65711 has finished for PR 15179 at commit 0c7e552.

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

[hhbyyh]

Hi @srowen. By fixing Word2Vec, do you mean to change its input type to Array(String, false)?

I cannot recall all the details. Right now I think the primary reason is to be consistent with auto schema inference, thus to support usage like https://github.com/apache/spark/blob/180fd3e0a3426db200c97170926afb60751dfd0e/examples/src/main/scala/org/apache/spark/examples/ml/Word2VecExample.scala

[srowen]

Or really to have it accept an array of strings, whether nullable or not. Right now it can accept a nullable string array type, but will pointlessly reject input that is not nullable. The check is too strict. We could just loosen that and then any string array input would work, which seems like the idea?

[hhbyyh]

I'm not sure what will happen if we actually send an Array(String, false) column to Word2Vec. If nothing goes wrong, I think we can loosen the constraint.

I'll try something locally.

[hhbyyh]

Checked locally and Word2Vec can work with Array(String, false). We can loosen the constraint. Maybe a new UT can be added.

[srowen]

I think the problem is that it won't accept anything that outputs Array(String, true)? it seems like it's looking for exactly the same type. Elsewhere I see a similar check written so as to not have to match nullability.

[hhbyyh]

CountVectorizer can accept both Array(String, true) and Array(String, false). Perhaps we can just change the validation code.

[srowen]

Yes that's better IMHO. It makes this more consistent. Word2Vec was the only case that looked for an exact match on nullability when it was expecting nullable input (i.e. pointlessly disallowed non-null input)

[SparkQA]

Test build #65769 has finished for PR 15179 at commit 4335237.

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

[hhbyyh]

LGTM. How about adding an UT for the input type new ArrayType(StringType, false)?

[srowen]

Sorry, what's a UT here? user defined type?

[hhbyyh]

Sorry, I meant unit test

[jkbradley]

Accepting more input types SGTM too (with unit tests). The PR title and description (and perhaps the JIRA too) should be updated. Thanks!

[SparkQA]

Test build #65821 has finished for PR 15179 at commit 76af236.

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

[hhbyyh]

Thanks Sean. LGTM.

[srowen]

Merged to master/2.0