[SPARK-27905] [SQL] Add higher order function 'forall' by nvander1 · Pull Request #24761 · apache/spark

nvander1 · 2019-05-31T18:10:11Z

What changes were proposed in this pull request?

Add's the higher order function forall, which tests an array to see if a predicate holds for every element.
The function is implemented in org.apache.spark.sql.catalyst.expressions.ArrayForAll.
The function is added to the function registry under the pretty name forall.

How was this patch tested?

I've added appropriate unit tests for the new ArrayForAll expression in
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HigherOrderFunctionsSuite.scala.

Also added tests for the function in sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala.

Not sure who is best to ask about this PR so:
@HyukjinKwon @rxin @gatorsmile @ueshin @srowen @hvanhovell @gatorsmile

HyukjinKwon · 2019-06-01T03:27:48Z

Do you know if any DBMS has this function?

srowen · 2019-06-01T12:55:08Z

I don't feel strongly about it. If exists is implemented, this seems reasonable to add, even if it's niche. I think you'd have to modify ReplaceNullWithFalseInPredicateSuite too? just looking at how ArrayExists works.

nvander1 · 2019-06-03T14:07:06Z

@srowen The latest commit should address that issue. Thanks for pointing it out! :)

srowen · 2019-06-03T14:21:28Z

@ueshin maybe you want to look as you added ArrayExists, etc

gatorsmile · 2019-06-03T21:30:58Z

ok to test

cc @hvanhovell

hvanhovell · 2019-06-04T07:46:59Z

ok to test

HyukjinKwon · 2019-06-04T07:55:08Z

@nvander1 can you clarify if there's any reference for this function? I am asking this for its name, behaviour and see if users actually need it.

hvanhovell · 2019-06-04T08:03:44Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

    assert(ex4.getMessage.contains("cannot resolve '`a`'"))
  }

+  test("forall function - array for primitive type not containing null") {


I think most/all of this should be covered by unit tests. You can add a single test to validate that the function registry works, if you must. I know you mirrored the tests for exists and I think they have the same problem. In general we should test the interface here (including the errors), and not so much the underlying functionality (that should be covered by UTs).

I agree with it, but I remember there was a problem that the behavior was different between the whole stage codegen on/off before, then we decided to do like this as a workaround (#21795).

ueshin

The implementation itself LGTM.
I'm not sure whether we need this or not yet. We had a short discussion before (SPARK-25068).
Also I'm wondering about the comment above (#24761 (comment)).

ueshin · 2019-06-04T10:03:54Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

    assert(ex4.getMessage.contains("cannot resolve '`a`'"))
  }

+  test("forall function - array for primitive type not containing null") {


I agree with it, but I remember there was a problem that the behavior was different between the whole stage codegen on/off before, then we decided to do like this as a workaround (#21795).

SparkQA · 2019-06-04T10:51:24Z

Test build #106139 has finished for PR 24761 at commit abef9cf.

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

rxin · 2019-06-05T09:58:13Z

Can we just logically rewrite this to use exists, rather than having two physical implementations?

nvander1 · 2019-06-07T14:23:26Z

@ueshin Re: the references for forall, its a fairly standard higher order function like exists, filter, map

https://www.scala-lang.org/api/current/scala/collection/IterableLike.html#forall(p:A=%3EBoolean):Boolean
https://www.scala-lang.org/api/current/scala/collection/IterableLike.html#exists(p:A=%3EBoolean):Boolean
https://www.scala-lang.org/api/current/scala/collection/IterableLike.html#filter(p:A=%3EBoolean):Repr

nvander1 · 2019-06-07T14:25:55Z

@ueshin re the whole stage code gen, do you meant to use this checkResult2 to test if the results are the same with and without it?

nvander1 · 2019-06-07T17:29:45Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+      i += 1
+    }
+    !check(continue)
+  }


I think the name for continue makes sense here, but I can see that the final !check(continue) returned value may be confusing to read. Anyone have suggestions to make the intent more clear here?

What is the intent of the !check(continue) ?

@yeikel perhaps the following implementation would be more clear?

override def nullSafeEval(inputRow: InternalRow, argumentValue: Any): Any = { val arr = argumentValue.asInstanceOf[ArrayData] val f = functionForEval var res = emptyRes var i = 0 while (!isConfirmed(res) && i < arr.numElements) { elementVar.value.set(arr.get(i, elementVar.dataType)) res = f.eval(inputRow).asInstanceOf[Boolean] i += 1 } res }

Where isConfirmed represents whether we can break out early from our while loop:
For ArrayExists, we can break out early as soon as we find an element that matches the predicate.
For ArrayForAll, we can break out early as soon as we find an element that does NOT match the predicate.

So for ArrayExists, we define the emptyRes to be false since there are no elements in the array to satisfy the predicate. And we define the isConfirmed to be just the result of the predicate on the most recent element.

For ArrayForAll, we define the emptyRes to be true since the predicate holds for every element of an empty array. And we define the isConfirmed to be the negation of the result of the predicate on the most recent element.

This is similar to the approach employed by the scala stdlib: https://github.com/scala/scala/blob/v2.13.0/src/library/scala/collection/IterableOnce.scala#L587-L606

Although they do not abstract out the operation over forall and exists. I'm all for keeping the code DRY like @rxin 's suggestion prompted, but if we can't find a way to do so that is easy to understand, maybe we should just have two implementations that are similar.

Here is a branch that I can merge into this one if needed with the changes I described above:
https://github.com/nvander1/spark/commit/aa5c94f5fb5ce9d677a65af7184c35752d2ca491

nvander1 · 2019-06-07T17:31:28Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+  extends ArrayExistsForAllBase {
+  override def prettyName: String = "forall"
+  override def check(cond: Boolean): Boolean = !cond
+  override def bind(f: (Expression, Seq[(DataType, Boolean)]) => LambdaFunction): ArrayForAll = {


I tried to factor out the bind definition as well here, but there seems to be a known issue on trying to rely on the generated copy method of a case class in its parent trait: https://www.scala-lang.org/old/node/6369

Aside: this is also the same bind method for ArrayFilter

nvander1 · 2019-06-07T17:51:41Z

@rxin Refactored so ArrayExists and ArrayForAll share an implementation

SparkQA · 2019-06-07T20:32:41Z

Test build #106278 has finished for PR 24761 at commit 1775c03.

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

HyukjinKwon · 2019-06-10T13:06:59Z

re: #24761 (comment) i'm okie with this.

gatorsmile · 2019-06-11T07:59:50Z

cc @ueshin

ueshin · 2019-06-14T06:50:55Z

@nvander1 I'm sorry for the delay.
Actually I had an offline discussion about this, and during the discussion we found that ArrayExists (also ArrayForAll if we provide) should follow the three-valued boolean logic.

After the discussion, I became okay to provide this since, even if the rewrite under the three-valued boolean logic works, it's easier and more confident for users.

Let me fix it first, then, although I'm not sure whether we can still share an implementation between the two, could you follow the fix here as well?

Thanks.

ueshin · 2019-06-14T07:15:26Z

I submitted a PR #24873.

ueshin · 2019-06-15T23:11:50Z

@nvander1 My PR #24873 was merged.
Could you continue this PR and follow the three-valued logic for forall as well please?

false if the predicate holds at least one false
otherwise, null if the predicate holds null
otherwise, true

See also: https://www.postgresql.org/docs/9.6/functions-comparisons.html#AEN21197

Thanks!

nvander1 · 2019-06-16T17:43:17Z

@ueshin Should there be a conf setting for following three valued logic on forall as well, or should it always be three-valued since there isn't any legacy code that would expect that?

~~Also, should we open another PR to make the ArrayFilter + MapFilter follow three valued logic for their predicates as well?~~ Now that I think about it, that wouldn't make much sense in that context.

~~Also, in the case of forall([1, null, 3], x -> x % 2 = 0) Should this evaluate null or false?~~ I see why this should be null now

Also apply 3 valued logic to ArrayForAll

nvander1 · 2019-06-16T21:57:36Z

Added in the three valued logic to ArrayForAll.

SparkQA · 2019-06-17T00:09:59Z

Test build #106562 has finished for PR 24761 at commit 04822ff.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

nvander1 · 2019-06-17T00:57:17Z

@rxin @ueshin @gatorsmile @HyukjinKwon @hvanhovell

I don't think the build failure is related to my changes. It is a failure in a spark streaming fault tolerance test

viirya · 2019-06-17T00:43:35Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+      > SELECT _FUNC_(array(2, 4, 8), x -> x % 2 == 0);
+       true
+      > SELECT _FUNC_(array(1, null, 3), x -> x % 2 == 0);
+       NULL


if the right-hand array contains any null elements and no false comparison result is obtained, the result of ALL will be null, not true

presto> SELECT 1 > ALL (VALUES 2, null, 3); _col0 ------- false (1 row)

The result of last example isn't false?

yeah, it should be false.

For example,

SELECT _FUNC_(array(2, null, 8), x -> x % 2 == 0);

should be null.

ueshin

@nvander1 Thanks for updating.
I don't think we need the config for this, and forall should always follow three-valued logic.
I left some comments. Could you update this please?
Thanks.

ueshin · 2019-06-17T02:22:05Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+    var forall = true
+    var foundNull = false
+    var i = 0
+    while (i < arr.numElements && (forall || !foundNull)) {


We can't break with foundNull since there is still a false after null was found. We need to find false first even if we found null.

ueshin · 2019-06-17T02:31:02Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+      null
+    } else {
+      forall
+    }


if (!forall) { false } else if (foundNull) { null } else { true }

?

ueshin · 2019-06-17T02:31:08Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala

+      > SELECT _FUNC_(array(2, 4, 8), x -> x % 2 == 0);
+       true
+      > SELECT _FUNC_(array(1, null, 3), x -> x % 2 == 0);
+       NULL


yeah, it should be false.

For example,

SELECT _FUNC_(array(2, null, 8), x -> x % 2 == 0);

should be null.

nvander1 · 2019-07-31T18:11:05Z

Should this be null or false?

SELECT _FUNC_(array(1, null, 3), x -> false)

It seems that this would be false, but following your suggested

if (!forall) {
  false
} else if (foundNull) {
  null
} else {
  true
}

would result in null.

@ueshin

nvander1 · 2019-07-31T18:21:35Z

I think this summarizes the expected behavior for forall with three valued logic:

  /*
   * true for all non null elements foundNull      result
   *    F                              F             F
   *    F                              T             F
   *    T                              F             T
   *    T                              T             N
   */

And this is captured by the following check

    if (foundNull && forall) {
      null
    } else {
      forall
    }

What do you think?

ueshin · 2019-07-31T20:25:24Z

It seems that this would be false, but following your suggested would result in null.

I don't think it returns null, but false properly.

Thanks for the summary, which is the expected behavior, and the logic should work as well.

ueshin · 2019-07-31T20:28:03Z

LGTM pending tests. cc @viirya

SparkQA · 2019-07-31T21:27:02Z

Test build #108490 has finished for PR 24761 at commit c93e701.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-08-02T02:26:39Z

Test build #108534 has finished for PR 24761 at commit af77619.

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

HyukjinKwon

Looks fine to me too but let me leave it to @ueshin

ueshin · 2019-08-06T17:28:07Z

I'm sorry for the delay.
Let me retrigger the tests since it's been a while since the last build.

ueshin · 2019-08-06T17:28:18Z

Jenkins, retest this please.

SparkQA · 2019-08-06T20:51:40Z

Test build #108728 has finished for PR 24761 at commit af77619.

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

ueshin · 2019-08-06T21:25:31Z

Thanks! merging to master.

### What changes were proposed in this pull request? This is a follow-up of #24761 which added a higher-order function `ArrayForAll`. The PR mistakenly removed the `prettyName` from `ArrayExists` and forgot to add it to `ArrayForAll`. ### Why are the changes needed? This reverts the `prettyName` back to `ArrayExists` not to affect explained plans, and adds it to `ArrayForAll` to clarify the `prettyName` as the same as the expressions around. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing tests. Closes #25501 from ueshin/issues/SPARK-27905/pretty_names. Authored-by: Takuya UESHIN <ueshin@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

Add higher order function 'forall'

9753f57

Handle ArrayForAll in ReplaceNullWithFalseInPredicate

abef9cf

hvanhovell reviewed Jun 4, 2019

View reviewed changes

ueshin reviewed Jun 4, 2019

View reviewed changes

Refactor ArrayExists + ArrayForAll to share implementation

1775c03

nvander1 commented Jun 7, 2019

View reviewed changes

dongjoon-hyun added the SQL label Jun 14, 2019

Merge branch 'master' into feature/for_all

04822ff

Also apply 3 valued logic to ArrayForAll

viirya reviewed Jun 17, 2019

View reviewed changes

ueshin reviewed Jun 17, 2019

View reviewed changes

Fix three valued logic for forall

c93e701

Correct expected test result

af77619

HyukjinKwon approved these changes Aug 2, 2019

View reviewed changes

nvander1 mentioned this pull request Aug 3, 2019

[SPARK-27297] [SQL] Add higher order functions to scala API #24232

Closed

ueshin closed this in 9e931e7 Aug 6, 2019

ueshin mentioned this pull request Aug 19, 2019

[SPARK-27905][SQL][FOLLOW-UP] Add prettyNames. #25501

Closed

Comments

Conversation

nvander1 commented May 31, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Jun 1, 2019

Uh oh!

srowen commented Jun 1, 2019

Uh oh!

nvander1 commented Jun 3, 2019

Uh oh!

srowen commented Jun 3, 2019

Uh oh!

gatorsmile commented Jun 3, 2019

Uh oh!

hvanhovell commented Jun 4, 2019

Uh oh!

HyukjinKwon commented Jun 4, 2019

Uh oh!

hvanhovell Jun 4, 2019 • 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!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 4, 2019

Uh oh!

rxin commented Jun 5, 2019

Uh oh!

nvander1 commented Jun 7, 2019

Uh oh!

nvander1 commented Jun 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvander1 Jun 12, 2019 • 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!

nvander1 commented Jun 7, 2019

Uh oh!

SparkQA commented Jun 7, 2019

Uh oh!

HyukjinKwon commented Jun 10, 2019

Uh oh!

gatorsmile commented Jun 11, 2019

Uh oh!

ueshin commented Jun 14, 2019

Uh oh!

ueshin commented Jun 14, 2019

Uh oh!

ueshin commented Jun 15, 2019

Uh oh!

nvander1 commented Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvander1 commented Jun 16, 2019

Uh oh!

SparkQA commented Jun 17, 2019

Uh oh!

nvander1 commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hvanhovell Jun 4, 2019 •

edited

Loading

nvander1 Jun 12, 2019 •

edited

Loading

nvander1 commented Jun 16, 2019 •

edited

Loading

nvander1 commented Jul 31, 2019 •

edited

Loading