Spark 3.4: Support NOT_EQ for V2 filters #7898

ConeyLiu · 2023-06-24T05:29:14Z

This splits the changes for NOT_EQ from #7886

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

szehon-ho

It looks mostly good to me, looks like some style nits

szehon-ho · 2023-06-27T17:29:40Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

            return null;
          }

-          if (predicate.name().equals(EQ)) {
-            // comparison with null in normal equality is always null. this is probably a mistake.
+          if (EQ.equals(predicate.name())) {


Probaby can move this to the top of the case as well, to avoid the unnecessary work if Precondition fails?

We should process the predicateChildren first, and then with the children to determine whether the value is null.

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

szehon-ho

Thanks for the changes, I read it again and sorry I had a few more questions and small comments, but looks pretty close to me though.

szehon-ho · 2023-06-30T07:46:46Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

    } else {
-      return equal(attribute, value);
+      return notEqual(children.first(), children.second());


I had a question , was this originally a bug? it was returning equal(string, value), not equal(NamedReference, value)

(My question is about the original code)

The string will be wrapped into NamedReference. There are two API as follows:

public static <T> UnboundPredicate<T> equal(String name, T value) { return new UnboundPredicate<>(Expression.Operation.EQ, ref(name), value); } public static <T> UnboundPredicate<T> equal(UnboundTerm<T> expr, T value) { return new UnboundPredicate<>(Expression.Operation.EQ, expr, value); }

I see, and you plan to pass UnboundTerm instead of string, for following pr

szehon-ho · 2023-06-30T07:47:49Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

+    }
+  }
+
+  private static UnboundPredicate<Object> handleNotEqual(


Sorry for not mentioning earlier, but can we make these two methods take not a Pair, but actual arguments, so this method itself is easier to read?

Then just invoke in the caller: , handle[Not]Equal(children.first(), children.second())

Good idea, updated for the methods to be more clearly.

szehon-ho · 2023-06-30T07:49:39Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

@@ -245,6 +251,22 @@ public static Expression convert(Predicate predicate) {
    return null;
  }

+  private static Pair<UnboundTerm<Object>, Object> predicateChildren(Predicate predicate) {
+    Object value;
+    UnboundTerm<Object> term;


Optional, my thought is , it would be clearer to have term be of type 'NamedReference', instead of super class 'UnboundTerm', not sure what you think? (because its more specific)

I would like this to be UnboundTerm because it could be a UnboundTransform as well in the following PRs.

szehon-ho

lgtm. Slightly prefer to not change literal String => UnboundTerm to make this diff smaller. But I get its needed in subsequent pr, not a big deal to me.

Will wait a little to see if further comments.

RussellSpitzer · 2023-06-30T22:28:13Z

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/TestSparkV2Filters.java

@@ -39,6 +39,7 @@

 public class TestSparkV2Filters {

+  @SuppressWarnings("checkstyle:MethodLength")


We should probably break this test up by operator :)

I'd be okay doing that in a follow-up change as well.

Split the internal test block to function testV2Filter.

RussellSpitzer · 2023-06-30T22:31:05Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

+  private static UnboundPredicate<Object> handleEqual(UnboundTerm<Object> term, Object value) {
+    if (value == null) {
+      return isNull(term);
+    }


minor style suggestion here, i find these a bit more clear as if, elseif, else

RussellSpitzer

Minor style suggestion about branching condition, (make branching explicit with elseIf instead of implied via early return)

I also think it would be great if we broke up the test method rather than putting in a suppression for the method length

aokolnychyi · 2023-07-01T00:19:54Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

+  private static UnboundPredicate<Object> handleNotEqual(UnboundTerm<Object> term, Object value) {
+    if (value == null) {
+      return notNull(term);
+    }


Minor: Same here, let's use if/else if/else.

aokolnychyi · 2023-07-01T00:22:54Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

+      term = ref(SparkUtil.toColumnName(rightChild(predicate)));
+      value = convertLiteral(leftChild(predicate));
+    } else {
+      return null;


Optional: I am not sure there is a lot of value in keeping term and value outside given that we have a nested return and not using them anywhere below. I'd probably format it like this, it up to you, though:

if (isRef(leftChild(predicate)) && isLiteral(rightChild(predicate))) { UnboundTerm<Object> term = ref(SparkUtil.toColumnName(leftChild(predicate))); Object value = convertLiteral(rightChild(predicate)); return Pair.of(term, value); } else if (isRef(rightChild(predicate)) && isLiteral(leftChild(predicate))) { UnboundTerm<Object> term = ref(SparkUtil.toColumnName(rightChild(predicate))); Object value = convertLiteral(leftChild(predicate)); return Pair.of(term, value); } else { return null; }

aokolnychyi

I echo Russell's comments around nested if blocks. Otherwise, seems correct to me.

szehon-ho

I think comments have been addressed, I think we could revert the unnecessary (to me) test change and then it should be ready to commit

szehon-ho · 2023-07-03T17:53:13Z

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/TestSparkV2Filters.java

+    attrMap.forEach(this::testV2Filter);
+  }
+
+  private void testV2Filter(String quoted, String unquoted) {


I think this is a bit hacky to avoid the length warning, as it doesnt add the readability more than the suppression does. I think @RussellSpitzer may have meant individual test methods, like:

@Test public void testIsNullV2Filter() { } @Test public void testIsNotNullV2Filter { }

Maybe we can revert it to what you had, and we can make a cleanup issue for this?

Oh sorry for the misunderstood. Reverted and will address it in a follow-up.

rdblue · 2023-07-03T22:44:30Z

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java

+
+  private static UnboundPredicate<Object> handleNotEqual(UnboundTerm<Object> term, Object value) {
+    if (value == null) {
+      return notNull(term);


<> is not null safe right? So this should return null or throw an exception if the value is null.

Sorry for missing that. Updated.

rdblue · 2023-07-03T22:46:11Z

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/TestSparkV2Filters.java

+    Assert.assertEquals(
+        "NotEqualTo must match", expectedNotEq1.toString(), actualNotEq1.toString());
+
+    Predicate notEq2 = new Predicate("<>", valueAndAttr);


Are there cases for null and NaN values? I think there should be since there is special handling for them.

rdblue · 2023-07-04T21:03:49Z

Thanks, @ConeyLiu! Now that this is in we can finish reviewing #7886.

ConeyLiu · 2023-07-05T14:40:51Z

Thanks @rdblue @szehon-ho @RussellSpitzer @aokolnychyi !

github-actions bot added the spark label Jun 24, 2023

ConeyLiu mentioned this pull request Jun 24, 2023

Spark 3.4: Support pushing down system functions by V2 filters #7886

Merged

szehon-ho reviewed Jun 24, 2023

View reviewed changes

ConeyLiu commented Jun 24, 2023

View reviewed changes

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 27, 2023

View reviewed changes

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java Outdated Show resolved Hide resolved

szehon-ho reviewed Jun 27, 2023

View reviewed changes

rdblue reviewed Jun 27, 2023

View reviewed changes

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 27, 2023

View reviewed changes

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java Outdated Show resolved Hide resolved

szehon-ho reviewed Jun 29, 2023

View reviewed changes

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java Outdated Show resolved Hide resolved

spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java Outdated Show resolved Hide resolved

szehon-ho reviewed Jun 30, 2023

View reviewed changes

szehon-ho approved these changes Jun 30, 2023

View reviewed changes

RussellSpitzer reviewed Jun 30, 2023

View reviewed changes

RussellSpitzer approved these changes Jun 30, 2023

View reviewed changes

aokolnychyi reviewed Jul 1, 2023

View reviewed changes

aokolnychyi approved these changes Jul 1, 2023

View reviewed changes

szehon-ho reviewed Jul 3, 2023

View reviewed changes

rdblue reviewed Jul 3, 2023

View reviewed changes

ConeyLiu added 8 commits July 4, 2023 20:04

Support NOT_EQ for V2 filters

1f96e74

address comments

0b7558a

address comments

67f53fc

address comments

f714fbf

address comments

45ecd1c

address comments

0352258

address comments

299c982

address comments

ae4cdbc

ConeyLiu force-pushed the not-eq-v2-filter branch from b8cf269 to ae4cdbc Compare July 4, 2023 15:37

rdblue approved these changes Jul 4, 2023

View reviewed changes

rdblue merged commit b8102d5 into apache:master Jul 4, 2023

ConeyLiu deleted the not-eq-v2-filter branch July 5, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spark 3.4: Support NOT_EQ for V2 filters #7898

Spark 3.4: Support NOT_EQ for V2 filters #7898

ConeyLiu commented Jun 24, 2023

szehon-ho left a comment

szehon-ho Jun 27, 2023

ConeyLiu Jun 28, 2023

szehon-ho left a comment

szehon-ho Jun 30, 2023

szehon-ho Jun 30, 2023

ConeyLiu Jun 30, 2023

szehon-ho Jun 30, 2023

szehon-ho Jun 30, 2023 •

edited

Loading

ConeyLiu Jun 30, 2023

szehon-ho Jun 30, 2023 •

edited

Loading

ConeyLiu Jun 30, 2023

szehon-ho left a comment •

edited

Loading

RussellSpitzer Jun 30, 2023

aokolnychyi Jul 1, 2023

ConeyLiu Jul 1, 2023

RussellSpitzer Jun 30, 2023

aokolnychyi Jul 1, 2023

ConeyLiu Jul 1, 2023

RussellSpitzer left a comment

aokolnychyi Jul 1, 2023

ConeyLiu Jul 1, 2023

aokolnychyi Jul 1, 2023

ConeyLiu Jul 1, 2023

aokolnychyi left a comment

szehon-ho left a comment

szehon-ho Jul 3, 2023 •

edited

Loading

ConeyLiu Jul 4, 2023

rdblue Jul 3, 2023

ConeyLiu Jul 4, 2023 •

edited

Loading

rdblue Jul 3, 2023

ConeyLiu Jul 4, 2023

rdblue commented Jul 4, 2023

ConeyLiu commented Jul 5, 2023

		@@ -39,6 +39,7 @@

		public class TestSparkV2Filters {

		@SuppressWarnings("checkstyle:MethodLength")

Spark 3.4: Support NOT_EQ for V2 filters #7898

Spark 3.4: Support NOT_EQ for V2 filters #7898

Conversation

ConeyLiu commented Jun 24, 2023

szehon-ho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szehon-ho left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi left a comment

Choose a reason for hiding this comment

szehon-ho left a comment

Choose a reason for hiding this comment

szehon-ho Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConeyLiu Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Jul 4, 2023

ConeyLiu commented Jul 5, 2023

szehon-ho Jun 30, 2023 •

edited

Loading

szehon-ho Jun 30, 2023 •

edited

Loading

szehon-ho left a comment •

edited

Loading

szehon-ho Jul 3, 2023 •

edited

Loading

ConeyLiu Jul 4, 2023 •

edited

Loading