Copilot commented on code in PR #15833:
URL: https://github.com/apache/iceberg/pull/15833#discussion_r3014411906
##########
api/src/test/java/org/apache/iceberg/transforms/TestProjection.java:
##########
@@ -158,77 +181,54 @@ public void testCaseSensitiveIdentityProjection() {
.hasMessageContaining("Cannot find field 'ID' in struct");
}
- @Test
- public void testStrictIdentityProjection() {
- List<UnboundPredicate<?>> predicates =
- Lists.newArrayList(
- Expressions.notNull("id"),
- Expressions.isNull("id"),
- Expressions.lessThan("id", 100),
- Expressions.lessThanOrEqual("id", 101),
- Expressions.greaterThan("id", 102),
- Expressions.greaterThanOrEqual("id", 103),
- Expressions.equal("id", 104),
- Expressions.notEqual("id", 105));
-
+ @ParameterizedTest
+ @MethodSource("predicatesWithReference")
+ public void testStrictIdentityProjection(UnboundPredicate<?> predicate) {
PartitionSpec spec =
PartitionSpec.builderFor(SCHEMA).identity("id").build();
- for (UnboundPredicate<?> predicate : predicates) {
- // get the projected predicate
- Expression expr = Projections.strict(spec).project(predicate);
- UnboundPredicate<?> projected = assertAndUnwrapUnbound(expr);
-
- // check inclusive the bound predicate to ensure the types are correct
- BoundPredicate<?> bound =
assertAndUnwrap(predicate.bind(spec.schema().asStruct(), true));
-
- assertThat(projected.ref().name())
- .as("Field name should match partition struct field")
- .isEqualTo("id");
- assertThat(projected.op()).isEqualTo(bound.op());
-
- if (bound.isLiteralPredicate()) {
- assertThat(projected.literal().value())
- .isEqualTo(bound.asLiteralPredicate().literal().value());
- } else {
- assertThat(projected.literal()).isNull();
- }
+ // get the projected predicate
+ Expression expr = Projections.strict(spec).project(predicate);
+ UnboundPredicate<?> projected = assertAndUnwrapUnbound(expr);
+
+ // check inclusive the bound predicate to ensure the types are correct
+ BoundPredicate<?> bound =
assertAndUnwrap(predicate.bind(spec.schema().asStruct(), true));
+
+ assertThat(projected.ref().name())
+ .as("Field name should match partition struct field")
+ .isEqualTo("id");
+ assertThat(projected.op()).isEqualTo(bound.op());
+
+ if (bound.isLiteralPredicate()) {
+ assertThat(projected.literal().value())
+ .isEqualTo(bound.asLiteralPredicate().literal().value());
+ } else {
+ assertThat(projected.literal()).isNull();
}
}
- @Test
- public void testCaseInsensitiveStrictIdentityProjection() {
- List<UnboundPredicate<?>> predicates =
- Lists.newArrayList(
- Expressions.notNull("ID"),
- Expressions.isNull("ID"),
- Expressions.lessThan("ID", 100),
- Expressions.lessThanOrEqual("ID", 101),
- Expressions.greaterThan("ID", 102),
- Expressions.greaterThanOrEqual("ID", 103),
- Expressions.equal("ID", 104),
- Expressions.notEqual("ID", 105));
+ @ParameterizedTest
+ @MethodSource("predicatesWithReference")
+ public void testCaseInsensitiveStrictIdentityProjection(UnboundPredicate<?>
predicate) {
PartitionSpec spec =
PartitionSpec.builderFor(SCHEMA).identity("id").build();
- for (UnboundPredicate<?> predicate : predicates) {
- // get the projected predicate
- Expression expr = Projections.strict(spec, false).project(predicate);
- UnboundPredicate<?> projected = assertAndUnwrapUnbound(expr);
-
- // check inclusive the bound predicate to ensure the types are correct
- BoundPredicate<?> bound =
assertAndUnwrap(predicate.bind(spec.schema().asStruct(), false));
-
- assertThat(projected.ref().name())
- .as("Field name should match partition struct field")
- .isEqualTo("id");
- assertThat(projected.op()).isEqualTo(bound.op());
-
- if (bound.isLiteralPredicate()) {
- assertThat(projected.literal().value())
- .isEqualTo(bound.asLiteralPredicate().literal().value());
- } else {
- assertThat(projected.literal()).isNull();
- }
+ // get the projected predicate
+ Expression expr = Projections.strict(spec, false).project(predicate);
+ UnboundPredicate<?> projected = assertAndUnwrapUnbound(expr);
+
+ // check inclusive the bound predicate to ensure the types are correct
+ BoundPredicate<?> bound =
assertAndUnwrap(predicate.bind(spec.schema().asStruct(), false));
+
Review Comment:
Same issue here: `@MethodSource("predicatesWithReference")` supplies
predicates on "id" (lowercase), so this test doesn't actually verify the
case-insensitive path. Use predicates referencing "ID" (uppercase) or include
both variants so the test would fail if case-insensitive resolution regressed.
##########
api/src/test/java/org/apache/iceberg/transforms/TestProjection.java:
##########
@@ -113,40 +148,28 @@ public void testTimestampNanosIdentityProjection() {
.isEqualTo(1658837594123456789L);
}
- @Test
- public void testCaseInsensitiveIdentityProjection() {
- List<UnboundPredicate<?>> predicates =
- Lists.newArrayList(
- Expressions.notNull("ID"),
- Expressions.isNull("ID"),
- Expressions.lessThan("ID", 100),
- Expressions.lessThanOrEqual("ID", 101),
- Expressions.greaterThan("ID", 102),
- Expressions.greaterThanOrEqual("ID", 103),
- Expressions.equal("ID", 104),
- Expressions.notEqual("ID", 105));
-
+ @ParameterizedTest
+ @MethodSource("predicatesWithReference")
+ public void testCaseInsensitiveIdentityProjection(UnboundPredicate<?>
predicate) {
PartitionSpec spec =
PartitionSpec.builderFor(SCHEMA).identity("id").build();
- for (UnboundPredicate<?> predicate : predicates) {
- // get the projected predicate
- Expression expr = Projections.inclusive(spec, false).project(predicate);
- UnboundPredicate<?> projected = assertAndUnwrapUnbound(expr);
-
- // check inclusive the bound predicate to ensure the types are correct
- BoundPredicate<?> bound =
assertAndUnwrap(predicate.bind(spec.schema().asStruct(), false));
-
- assertThat(projected.ref().name())
- .as("Field name should match partition struct field")
- .isEqualTo("id");
- assertThat(projected.op()).isEqualTo(bound.op());
-
- if (bound.isLiteralPredicate()) {
- assertThat(projected.literal().value())
- .isEqualTo(bound.asLiteralPredicate().literal().value());
- } else {
- assertThat(projected.literal()).isNull();
- }
+ // get the projected predicate
+ Expression expr = Projections.inclusive(spec, false).project(predicate);
+ UnboundPredicate<?> projected = assertAndUnwrapUnbound(expr);
+
+ // check inclusive the bound predicate to ensure the types are correct
+ BoundPredicate<?> bound =
assertAndUnwrap(predicate.bind(spec.schema().asStruct(), false));
Review Comment:
These case-insensitive projection tests no longer exercise case-insensitive
name matching because `predicatesWithReference()` uses the lowercase column
name "id". As a result, the test would pass even if case-insensitive handling
were broken. Consider using predicates built with "ID" (or parameterize over
both casings) when `caseSensitive=false` to actually validate the behavior.
```suggestion
// rebuild the predicate to use an upper-case field name to validate
case-insensitive matching
UnboundPredicate<?> caseInsensitivePredicate;
if (predicate.isUnaryPredicate()) {
caseInsensitivePredicate = Expressions.predicate(predicate.op(), "ID");
} else if (predicate.isLiteralPredicate()) {
caseInsensitivePredicate =
Expressions.predicate(predicate.op(), "ID", predicate.literal());
} else {
caseInsensitivePredicate =
Expressions.predicate(predicate.op(), "ID", predicate.literals());
}
// get the projected predicate
Expression expr = Projections.inclusive(spec,
false).project(caseInsensitivePredicate);
UnboundPredicate<?> projected = assertAndUnwrapUnbound(expr);
// check inclusive the bound predicate to ensure the types are correct
BoundPredicate<?> bound =
assertAndUnwrap(caseInsensitivePredicate.bind(spec.schema().asStruct(), false));
```
--
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]
