Introduce startsWith Predicate #327

sujithjay · 2019-07-29T18:46:25Z

Implements #31.

sujithjay · 2019-07-29T18:46:44Z

@renato2099 & @Liorba had submitted PRs previously for this issue. I am taking up on their work, and trying to see if it can shepherded into master.

sujithjay · 2019-07-29T18:48:15Z

@rdblue Could you please take a look? Thank you.

sujithjay · 2019-07-29T18:51:03Z

api/src/main/java/org/apache/iceberg/transforms/Bucket.java

@@ -113,6 +116,7 @@ public String toString() {
            predicate.op(), name, apply(predicate.literal().value()));
 //      case IN:
 //        return Expressions.predicate();
+      case STARTS_WITH:


I am not sure if a startsWith predicate makes sense in case of Bucket transforms. Hence, leaving it to be handled by the default case. WDYT?

Agreed. Transformed values tell us nothing about whether the original predicate is true or not.

api/src/main/java/org/apache/iceberg/transforms/Truncate.java

rdblue · 2019-08-03T22:42:27Z

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java

+    assertProjectionInclusive(spec, startsWith("someStringCol", "ababab"), "abab");
+
+    assertProjectionStrict(spec, startsWith("someStringCol", "ab"), "ab");
+    assertProjectionStrict(spec, startsWith("someStringCol", "abab"), "abab");


I think this should also test strict projection for startsWith("someStringCol", "ababab") since that doesn't have a strict projection.

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java

rdblue · 2019-08-03T22:45:33Z

api/src/main/java/org/apache/iceberg/transforms/Truncate.java

-      return null;
+      switch (predicate.op()) {
+        case STARTS_WITH:
+          if (predicate.literal().value().length() <= width()) {


For the case where the length of the literal equals width, is there value in converting to an equality predicate?

I have converted it into an equality predicate.

rdblue · 2019-08-03T22:46:55Z

@sujithjay, nice work! I think this is about ready to go. I just had a few comments.

@moulimukherjee, could you take a look at this as well, since you're familiar with the projections?

Co-authored-by: Renato Marroquin <[email protected]> Co-authored-by: Lior Baber <[email protected]> Co-authored-by: Sujith Jay Nair <[email protected]>

sujithjay · 2019-08-07T18:59:22Z

api/src/main/java/org/apache/iceberg/transforms/Truncate.java

+          } else if (predicate.literal().value().length() == width()) {
+            return Expressions.equal(name, predicate.literal().value());
+          } else {
+            return null;


Does returning null here still make sense, in light of recent changes? Or should it be returning ProjectionUtil.truncateArrayStrict(name, predicate, this) ?

Returning ProjectionUtil.truncateArrayStrict(name, predicate, this) would still return null anyways, since it can't handle the STARTS_WITH case. So I think it makes sense to return null here.

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java

moulimukherjee · 2019-08-07T19:45:47Z

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java

+  }
+
+  @Test
+  public void assertTruncateString() {


Can some tests be added in the TestTruncatesResiduals.testStringTruncateTransformResiduals() too with the STARTS_WITH?

ExpressionVisitors for startsWith currently throw an UnsupportedOperationException (c.f. discussion-thread).

Given that, tests in TestTruncatesResiduals with STARTS_WITH would not add much value. What do you think?

@sujithjay, I agree with @moulimukherjee. But you're right that currently an UnsupportedOperationException will be thrown. That's a problem because it will prevent creating splits for a scan when there is a startsWith predicate.

The fix is to implement startsWith in ResidualEvaluator to support residual calculations and to add a couple of tests.

+1 for ^ (implement startsWith in ResidualEvaluator and add tests)

moulimukherjee · 2019-08-07T19:45:55Z

Some nits, but the changes look good to me 👍.

moulimukherjee · 2019-08-09T23:45:23Z

api/src/test/java/org/apache/iceberg/transforms/TestTruncatesResiduals.java

+    // starts with
+    assertResidualValue(spec, startsWith("value", "bcd"), "ab", Expression.Operation.FALSE);
+    assertResidualPredicate(spec, startsWith("value", "bcd"), "bc");
+    assertResidualValue(spec, startsWith("value", "bcd"), "cd", Expression.Operation.FALSE);


sujithjay · 2019-08-12T08:12:31Z

@rdblue, @moulimukherjee I have updated the PR. Please review.

moulimukherjee · 2019-08-12T17:58:51Z

Changes look good to me 👍

rdblue · 2019-08-12T18:05:28Z

api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java

@@ -120,6 +124,9 @@ public R or(R leftResult, R rightResult) {
          return in(pred.ref(), pred.literal());
        case NOT_IN:
          return notIn(pred.ref(), pred.literal());
+        case STARTS_WITH:
+          /* startsWith accepts only Strings, hence type-casting */
+          return startsWith((BoundReference<String>) pred.ref(), (Literal<String>) pred.literal());


I didn't catch this the first time through, but if the startsWith method above is parameterized by T then there shouldn't be a need to cast these to literals and bound references parameterized by String.

startsWith was parameterised with T, but I missed removing the casts here.

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java

rdblue · 2019-08-12T18:09:55Z

@sujithjay, I just found 2 minor issues and I'll merge this after those are fixed. Thanks for pushing this feature through!

sujithjay · 2019-08-12T18:52:41Z

Thank you for the help, @rdblue & @moulimukherjee. 🙏

aokolnychyi · 2019-08-20T16:22:11Z

I would like to follow up with a PR to enable pushdown of startsWith in Spark. Are there any limitations that will block me?

Apart from changes in SparkFilters, we will need to extend InclusiveMetricsEvaluator, ParquetDictionaryRowGroupFilter, ParquetMetricsRowGroupFilter with logic that checks lower/upper bounds in startsWith.

I think one way to implement this is to take the same number of bytes from prefix and bounds (e.g. slice) and then rely on UnsignedByteBufComparator for unsigned lexicographical comparison. Alternatively, we can convert bounds to strings and operate on them.

@sujithjay @rdblue @moulimukherjee Any thoughts?

rdblue · 2019-08-20T16:39:06Z

That plan sounds good to me!

… filed in apache#2019 )

sujithjay force-pushed the ICEBERG-31 branch from 29e5e48 to 863c8e9 Compare July 29, 2019 18:54

sujithjay commented Jul 29, 2019

View reviewed changes

sujithjay force-pushed the ICEBERG-31 branch 6 times, most recently from a3c38b7 to 3180cec Compare July 29, 2019 21:22

rdblue reviewed Aug 3, 2019

View reviewed changes

api/src/main/java/org/apache/iceberg/transforms/Truncate.java Outdated Show resolved Hide resolved

rdblue reviewed Aug 3, 2019

View reviewed changes

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java Outdated Show resolved Hide resolved

rdblue reviewed Aug 3, 2019

View reviewed changes

sujithjay force-pushed the ICEBERG-31 branch from 283dc06 to 61b0ef9 Compare August 7, 2019 18:47

renato2099 and others added 3 commits August 7, 2019 20:49

Previous Work

5404282

Co-authored-by: Renato Marroquin <[email protected]> Co-authored-by: Lior Baber <[email protected]> Co-authored-by: Sujith Jay Nair <[email protected]>

Add Projections

7068a9f

Address Review Comments

ccf6e5b

sujithjay force-pushed the ICEBERG-31 branch from 61b0ef9 to ccf6e5b Compare August 7, 2019 18:56

sujithjay commented Aug 7, 2019

View reviewed changes

moulimukherjee reviewed Aug 7, 2019

View reviewed changes

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java Outdated Show resolved Hide resolved

moulimukherjee reviewed Aug 7, 2019

View reviewed changes

sujithjay added 2 commits August 7, 2019 22:40

Rename Tests

52b1162

Implement startsWith in ResidualEvaluator; Add Tests

c45a767

moulimukherjee reviewed Aug 9, 2019

View reviewed changes

rdblue reviewed Aug 12, 2019

View reviewed changes

api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java Outdated Show resolved Hide resolved

sujithjay added 2 commits August 12, 2019 20:41

Remove Needless Cast

ab39869

Minor Edits to TestStartsWith

a9fb3a5

rdblue merged commit 5cfc119 into apache:master Aug 12, 2019

rdblue mentioned this pull request Aug 12, 2019

Add startsWith predicate #75

Closed

sujithjay deleted the ICEBERG-31 branch August 12, 2019 19:01

xabriel mentioned this pull request Oct 9, 2019

Add startsWith predicate #31

Closed

waterlx mentioned this pull request Jan 17, 2020

Add Parquet user-defined predicate to push down StartsWith #682

Closed

holdenk added a commit to holdenk/incubator-iceberg that referenced this pull request Jun 29, 2021

Add an implemention of STARTS_WITH for Python (follow up for apache#327…

bb49047

… filed in apache#2019 )

holdenk mentioned this pull request Jun 29, 2021

Add an implemention of STARTS_WITH for Python API. #2761

Closed

holdenk added a commit to holdenk/incubator-iceberg that referenced this pull request Jul 23, 2021

Add an implemention of STARTS_WITH for Python (follow up for apache#327…

8562593

… filed in apache#2019 )

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce startsWith Predicate #327

Introduce startsWith Predicate #327

sujithjay commented Jul 29, 2019 •

edited

Loading

sujithjay commented Jul 29, 2019

sujithjay commented Jul 29, 2019

sujithjay Jul 29, 2019

rdblue Aug 3, 2019

rdblue Aug 3, 2019

rdblue Aug 3, 2019

sujithjay Aug 7, 2019 •

edited

Loading

rdblue commented Aug 3, 2019

sujithjay Aug 7, 2019

moulimukherjee Aug 7, 2019

moulimukherjee Aug 7, 2019

sujithjay Aug 7, 2019

rdblue Aug 7, 2019

moulimukherjee Aug 8, 2019

moulimukherjee commented Aug 7, 2019

moulimukherjee Aug 9, 2019

sujithjay commented Aug 12, 2019

moulimukherjee commented Aug 12, 2019

rdblue Aug 12, 2019

sujithjay Aug 12, 2019

rdblue commented Aug 12, 2019

sujithjay commented Aug 12, 2019

aokolnychyi commented Aug 20, 2019

rdblue commented Aug 20, 2019

Introduce startsWith Predicate #327

Introduce startsWith Predicate #327

Conversation

sujithjay commented Jul 29, 2019 • edited Loading

sujithjay commented Jul 29, 2019

sujithjay commented Jul 29, 2019

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

sujithjay Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

rdblue commented Aug 3, 2019

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

moulimukherjee commented Aug 7, 2019

Choose a reason for hiding this comment

sujithjay commented Aug 12, 2019

moulimukherjee commented Aug 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Aug 12, 2019

sujithjay commented Aug 12, 2019

aokolnychyi commented Aug 20, 2019

rdblue commented Aug 20, 2019

sujithjay commented Jul 29, 2019 •

edited

Loading

sujithjay Aug 7, 2019 •

edited

Loading