yangshangqing95 commented on code in PR #17906:
URL: https://github.com/apache/iceberg/pull/17906#discussion_r3927390439
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -182,6 +184,48 @@ public void testNotStartsWith() {
.isEqualTo(21);
}
+ @Test
+ public void testContains() {
Review Comment:
```
void contains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -348,6 +350,60 @@ public void testNotStartsWith() {
.isTrue();
}
+ @Test
+ public void testContains() {
+ StructType struct = StructType.of(required(24, "s",
Types.StringType.get()));
+ Evaluator evaluator = new Evaluator(struct, contains("s", "abc"));
+ assertThat(evaluator.eval(TestHelpers.Row.of("abc")))
+ .as("abc contains abc should be true")
+ .isTrue();
+ assertThat(evaluator.eval(TestHelpers.Row.of("xabc")))
+ .as("xabc contains abc should be true")
+ .isTrue();
+ assertThat(evaluator.eval(TestHelpers.Row.of("Abc")))
+ .as("Abc contains abc should be false")
+ .isFalse();
+ assertThat(evaluator.eval(TestHelpers.Row.of("a")))
+ .as("a contains abc should be false")
+ .isFalse();
+ assertThat(evaluator.eval(TestHelpers.Row.of("abcd")))
+ .as("abcd contains abc should be true")
+ .isTrue();
+ assertThat(evaluator.eval(TestHelpers.Row.of("xabcd")))
+ .as("xabcd contains abc should be true")
+ .isTrue();
+ assertThat(evaluator.eval(TestHelpers.Row.of((String) null)))
+ .as("null contains abc should be false")
+ .isFalse();
+ }
+
+ @Test
+ public void testNotContains() {
Review Comment:
```
void notContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java:
##########
@@ -373,6 +373,45 @@ public void testSanitizeNotStartsWith() {
.isEqualTo("data NOT STARTS WITH (hash-34d05fb7)");
}
+ @Test
+ public void testSanitizeContains() {
Review Comment:
```
void sanitizeContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java:
##########
@@ -373,6 +373,45 @@ public void testSanitizeNotStartsWith() {
.isEqualTo("data NOT STARTS WITH (hash-34d05fb7)");
}
+ @Test
+ public void testSanitizeContains() {
+ assertEquals(
+ Expressions.contains("test", "(hash-34d05fb7)"),
+ ExpressionUtil.sanitize(Expressions.contains("test", "aaa")));
+
+ assertEquals(
+ Expressions.contains("data", "(hash-34d05fb7)"),
+ ExpressionUtil.sanitize(STRUCT, Expressions.contains("data", "aaa"),
true));
+
+ assertThat(ExpressionUtil.toSanitizedString(Expressions.contains("test",
"aaa")))
+ .as("Sanitized string should be identical except for descriptive
literal")
+ .isEqualTo("test CONTAINS (hash-34d05fb7)");
+
+ assertThat(ExpressionUtil.toSanitizedString(STRUCT,
Expressions.contains("data", "aaa"), true))
+ .as("Sanitized string should be identical except for descriptive
literal")
+ .isEqualTo("data CONTAINS (hash-34d05fb7)");
+ }
+
+ @Test
+ public void testSanitizeNotContains() {
Review Comment:
```
void sanitizeNotContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluator.java:
##########
@@ -863,6 +865,59 @@ public void testStringNotStartsWith() {
assertThat(shouldRead).as("Should not read: lower shorter than prefix,
cannot match").isTrue();
}
+ @Test
+ public void testStringContains() {
+ boolean shouldRead = shouldRead(SCHEMA, contains("required", "a"), true,
file());
+ assertThat(shouldRead).as("Should read: no stats").isTrue();
+
+ shouldRead = shouldRead(SCHEMA, contains("required", "a"), true, file2());
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead = shouldRead(SCHEMA, contains("required", "aB"), true, file2());
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead = shouldRead(SCHEMA, contains("required", "dWX"), true,
file2());
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead = shouldRead(SCHEMA, contains("required", "5"), true, file3());
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead = shouldRead(SCHEMA, contains("required", "3str3x"), true,
file3());
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead = shouldRead(SCHEMA, contains("all_nulls", ""), true, file());
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ String aboveMax = UnicodeUtil.truncateStringMax(Literal.of("イロハニホヘト"),
4).value().toString();
+ shouldRead = shouldRead(SCHEMA, contains("required", aboveMax), true,
file4());
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+ }
+
+ @Test
+ public void testStringNotContains() {
Review Comment:
```
void stringNotContains()
```
##########
api/src/main/java/org/apache/iceberg/expressions/BoundLiteralPredicate.java:
##########
@@ -88,6 +88,10 @@ public boolean test(T value) {
return String.valueOf(value).startsWith((String) literal.value());
case NOT_STARTS_WITH:
return !String.valueOf(value).startsWith((String) literal.value());
+ case CONTAINS:
+ return String.valueOf(value).contains((String) literal.value());
+ case NOT_CONTAINS:
+ return !String.valueOf(value).contains((String) literal.value());
Review Comment:
Do we want to keep the evaluation semantics consistent between
BoundLiteralPredicate and Evaluator here?
Evaluator.contains explicitly handles null before calling String.contains,
while this uses `String.valueOf(value)`. If test can ever receive a null value,
those differ for literals such as "null".
I see `STARTS_WITH` already uses the `String.valueOf` pattern here, so this
may be pre-existing, but adding the new operators seems like a good point to
make sure the two evaluation paths have the same contract.
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -182,6 +184,48 @@ public void testNotStartsWith() {
.isEqualTo(21);
}
+ @Test
+ public void testContains() {
+ StructType struct = StructType.of(required(0, "s",
Types.StringType.get()));
+ Expression expr = contains("s", "abc");
+ Expression boundExpr = Binder.bind(struct, expr);
+ TestHelpers.assertAllReferencesBound("Contains", boundExpr);
+ // make sure the expression is a Contains
+ BoundPredicate<?> pred = TestHelpers.assertAndUnwrap(boundExpr,
BoundPredicate.class);
+ assertThat(pred.op()).as("Should be right
operation").isEqualTo(Expression.Operation.CONTAINS);
+ assertThat(pred.term().ref().fieldId()).as("Should bind s
correctly").isZero();
+ }
+
+ @Test
+ public void testNotContains() {
+ StructType struct = StructType.of(required(21, "s",
Types.StringType.get()));
+ Expression expr = notContains("s", "abc");
+ Expression boundExpr = Binder.bind(struct, expr);
+ TestHelpers.assertAllReferencesBound("NotContains", boundExpr);
+ // Make sure the expression is a NotContains
+ BoundPredicate<?> pred = TestHelpers.assertAndUnwrap(boundExpr,
BoundPredicate.class);
+ assertThat(pred.op())
+ .as("Should be right operation")
+ .isEqualTo(Expression.Operation.NOT_CONTAINS);
+ assertThat(pred.term().ref().fieldId())
+ .as("Should bind term to correct field id")
+ .isEqualTo(21);
+ }
+
+ @Test
+ public void testContainsNonStringColumn() {
Review Comment:
```
void containsNonStringColumn()
```
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -198,6 +198,14 @@ public static UnboundPredicate<String>
notStartsWith(String name, String value)
return new UnboundPredicate<>(Expression.Operation.NOT_STARTS_WITH,
ref(name), value);
}
+ public static UnboundPredicate<String> contains(String name, String value) {
+ return new UnboundPredicate<>(Expression.Operation.CONTAINS, ref(name),
value);
+ }
+
+ public static UnboundPredicate<String> notContains(String name, String
value) {
+ return new UnboundPredicate<>(Expression.Operation.NOT_CONTAINS,
ref(name), value);
+ }
+
Review Comment:
Is `CONTAINS` intentionally limited to direct column references?
If transformed string terms are intended to be supported as well, should we
add the corresponding `UnboundTerm<String>` overloads? Otherwise the current
API is fine, but it may be worth keeping the restriction intentional.
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -182,6 +184,48 @@ public void testNotStartsWith() {
.isEqualTo(21);
}
+ @Test
+ public void testContains() {
+ StructType struct = StructType.of(required(0, "s",
Types.StringType.get()));
+ Expression expr = contains("s", "abc");
+ Expression boundExpr = Binder.bind(struct, expr);
+ TestHelpers.assertAllReferencesBound("Contains", boundExpr);
+ // make sure the expression is a Contains
+ BoundPredicate<?> pred = TestHelpers.assertAndUnwrap(boundExpr,
BoundPredicate.class);
+ assertThat(pred.op()).as("Should be right
operation").isEqualTo(Expression.Operation.CONTAINS);
+ assertThat(pred.term().ref().fieldId()).as("Should bind s
correctly").isZero();
+ }
+
+ @Test
+ public void testNotContains() {
Review Comment:
```
void notContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -348,6 +350,60 @@ public void testNotStartsWith() {
.isTrue();
}
+ @Test
+ public void testContains() {
Review Comment:
nit: based on the latest iceberg code style, any new added test should
without the `test` prefix
```
void contains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -182,6 +184,48 @@ public void testNotStartsWith() {
.isEqualTo(21);
}
+ @Test
+ public void testContains() {
+ StructType struct = StructType.of(required(0, "s",
Types.StringType.get()));
+ Expression expr = contains("s", "abc");
+ Expression boundExpr = Binder.bind(struct, expr);
+ TestHelpers.assertAllReferencesBound("Contains", boundExpr);
+ // make sure the expression is a Contains
+ BoundPredicate<?> pred = TestHelpers.assertAndUnwrap(boundExpr,
BoundPredicate.class);
+ assertThat(pred.op()).as("Should be right
operation").isEqualTo(Expression.Operation.CONTAINS);
+ assertThat(pred.term().ref().fieldId()).as("Should bind s
correctly").isZero();
+ }
+
+ @Test
+ public void testNotContains() {
+ StructType struct = StructType.of(required(21, "s",
Types.StringType.get()));
+ Expression expr = notContains("s", "abc");
+ Expression boundExpr = Binder.bind(struct, expr);
+ TestHelpers.assertAllReferencesBound("NotContains", boundExpr);
+ // Make sure the expression is a NotContains
+ BoundPredicate<?> pred = TestHelpers.assertAndUnwrap(boundExpr,
BoundPredicate.class);
+ assertThat(pred.op())
+ .as("Should be right operation")
+ .isEqualTo(Expression.Operation.NOT_CONTAINS);
+ assertThat(pred.term().ref().fieldId())
+ .as("Should bind term to correct field id")
+ .isEqualTo(21);
+ }
+
+ @Test
+ public void testContainsNonStringColumn() {
+ assertThatThrownBy(() -> Binder.bind(STRUCT, contains("x", "abc")))
+ .isInstanceOf(ValidationException.class)
+ .hasMessageContaining("Term for CONTAINS must produce a string");
+ }
+
+ @Test
+ public void testNotContainsNonStringColumn() {
Review Comment:
```
void notContainsNonStringColumn()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveManifestEvaluator.java:
##########
@@ -663,6 +665,49 @@ public void testStringNotStartsWith() {
assertThat(shouldRead).as("Should not read: all values start with the
prefix").isFalse();
}
+ @Test
+ public void testStringContains() {
Review Comment:
```
void stringContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveManifestEvaluator.java:
##########
@@ -663,6 +665,49 @@ public void testStringNotStartsWith() {
assertThat(shouldRead).as("Should not read: all values start with the
prefix").isFalse();
}
+ @Test
+ public void testStringContains() {
+ boolean shouldRead =
+ ManifestEvaluator.forRowFilter(contains("some_nulls", "a"), SPEC,
false).eval(FILE);
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead =
+ ManifestEvaluator.forRowFilter(contains("some_nulls", "dddd"), SPEC,
false).eval(FILE);
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead = ManifestEvaluator.forRowFilter(contains("no_nulls", "a"),
SPEC, false).eval(FILE);
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead =
+ ManifestEvaluator.forRowFilter(contains("some_nulls", "zzzz"), SPEC,
false).eval(FILE);
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+
+ shouldRead =
+ ManifestEvaluator.forRowFilter(contains("some_nulls", "1"), SPEC,
false).eval(FILE);
+ assertThat(shouldRead).as("Should read: contains never skips").isTrue();
+ }
+
+ @Test
+ public void testStringNotContains() {
Review Comment:
```
void stringNotContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluator.java:
##########
@@ -863,6 +865,59 @@ public void testStringNotStartsWith() {
assertThat(shouldRead).as("Should not read: lower shorter than prefix,
cannot match").isTrue();
}
+ @Test
+ public void testStringContains() {
Review Comment:
```
void stringContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestStrictMetricsEvaluator.java:
##########
@@ -826,6 +828,45 @@ void testStartsWithNoStats() {
assertThat(shouldRead).as("Should not match: no bounds available for
column").isFalse();
}
+ @Test
+ void testContains() {
+ boolean shouldRead =
+ new StrictMetricsEvaluator(SCHEMA, contains("required",
"ab")).eval(STRING_FILE);
+ assertThat(shouldRead).as("Should not match: contains cannot be evaluated
strictly").isFalse();
+
+ shouldRead = new StrictMetricsEvaluator(SCHEMA, contains("required",
"a")).eval(STRING_FILE);
+ assertThat(shouldRead).as("Should not match: contains cannot be evaluated
strictly").isFalse();
+
+ shouldRead = new StrictMetricsEvaluator(SCHEMA, contains("required",
"zzz")).eval(STRING_FILE);
+ assertThat(shouldRead).as("Should not match: contains cannot be evaluated
strictly").isFalse();
+
+ shouldRead = new StrictMetricsEvaluator(SCHEMA, contains("required",
"a")).eval(FILE);
+ assertThat(shouldRead).as("Should not match: no bounds available for
column").isFalse();
+ }
+
+ @Test
+ void testNotContains() {
Review Comment:
```
void notContains()
```
##########
api/src/test/java/org/apache/iceberg/expressions/TestStrictMetricsEvaluator.java:
##########
@@ -826,6 +828,45 @@ void testStartsWithNoStats() {
assertThat(shouldRead).as("Should not match: no bounds available for
column").isFalse();
}
+ @Test
+ void testContains() {
Review Comment:
```
void contains()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
