uros-b commented on code in PR #17998:
URL: https://github.com/apache/iceberg/pull/17998#discussion_r3955862975
##########
orc/src/test/java/org/apache/iceberg/orc/TestExpressionToSearchArgument.java:
##########
@@ -221,6 +221,24 @@ public void testUnsupportedTypes() {
assertThat(actual.toString()).isEqualTo(expected.toString());
}
+ @Test
+ public void testVariantType() {
+ Schema schema =
+ new Schema(
+ required(1, "long", Types.LongType.get()),
+ optional(2, "variant", Types.VariantType.get()));
+
+ // predicates on other columns are still pushed down when the schema
contains a variant column
+ Expression expr = equal("long", 1);
+ Expression boundFilter = Binder.bind(schema.asStruct(), expr, true);
+ SearchArgument expected =
+ SearchArgumentFactory.newBuilder().startAnd().equals("`long`",
Type.LONG, 1L).end().build();
+
+ SearchArgument actual =
+ ExpressionToSearchArgument.convert(boundFilter,
ORCSchemaUtil.convert(schema));
+ assertThat(actual.toString()).isEqualTo(expected.toString());
+ }
+
Review Comment:
If we look at
`orc/src/main/java/org/apache/iceberg/orc/ExpressionToSearchArgument.java:59`,
`TypeID.VARIANT` is not in `UNSUPPORTED_TYPES`, so once this fix makes the
variant field visible to the predicate visitor, a unary isNull/notNull bound to
an optional variant column is no longer short-circuited to YES_NO_NULL; it
reaches super.predicate() -> isNull()/notNull() -> type(VariantType) (default
branch) -> UnsupportedOperationException.
This path is reachable end-to-end, Expressions.isNull("variant") on an
optional variant column binds to a real BoundUnaryPredicate(IS_NULL, ...) (in
UnboundPredicate.bindUnaryOperation the producesNull() guard is false for an
optional column, so it is not folded to alwaysFalse), and
OrcIterable.iterator() passes the bound scan filter straight into
ExpressionToSearchArgument.convert() with no residual rewrite that would drop
it, so table.newScan().filter(Expressions.isNull("variant")) on an ORC table
crashes at read time, no engine required.
It fits within this PR's stated scope (filter pushdown on variant-column
tables) and is an internal error vs clean error gap, so it is worth closing
here: please consider adding TypeID.VARIANT to UNSUPPORTED_TYPES (yielding
YES_NO_NULL, a correct no-op since ORC cannot evaluate a predicate on the
variant blob) and add an isNull("variant") test. Before this PR the same query
also threw (earlier, at map build), so it is not a regression, but a
newly-reachable crash the fix leaves one step short of its own goal.
--
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]
