JingsongLi commented on code in PR #9427:
URL: https://github.com/apache/paimon/pull/9427#discussion_r3912045910
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/PredicateConverter.java:
##########
@@ -74,74 +74,116 @@ public PredicateConverter(PredicateBuilder builder) {
@Override
public Predicate visit(CallExpression call) {
+ return visit(call, false);
+ }
+
+ private Predicate visit(CallExpression call, boolean negated) {
FunctionDefinition func = call.getFunctionDefinition();
List<Expression> children = call.getChildren();
if (func == BuiltInFunctionDefinitions.AND) {
- return PredicateBuilder.and(flattenAndConvert(children, func));
+ requireAtLeastArity(children, 2);
+ List<Predicate> predicates = flattenAndConvert(children, func,
negated);
+ return negated ? PredicateBuilder.or(predicates) :
PredicateBuilder.and(predicates);
} else if (func == BuiltInFunctionDefinitions.OR) {
- return PredicateBuilder.or(flattenAndConvert(children, func));
+ requireAtLeastArity(children, 2);
+ List<Predicate> predicates = flattenAndConvert(children, func,
negated);
+ return negated ? PredicateBuilder.and(predicates) :
PredicateBuilder.or(predicates);
+ } else if (func == BuiltInFunctionDefinitions.NOT) {
+ requireArity(children, 1);
+ return visit(children.get(0), !negated);
} else if (func == BuiltInFunctionDefinitions.EQUALS) {
- return visitBiFunction(children, builder::equal, builder::equal);
+ return negated
+ ? visitBiFunction(children, builder::notEqual,
builder::notEqual)
+ : visitBiFunction(children, builder::equal,
builder::equal);
} else if (func == BuiltInFunctionDefinitions.NOT_EQUALS) {
- return visitBiFunction(children, builder::notEqual,
builder::notEqual);
+ return negated
+ ? visitBiFunction(children, builder::equal, builder::equal)
+ : visitBiFunction(children, builder::notEqual,
builder::notEqual);
} else if (func == BuiltInFunctionDefinitions.GREATER_THAN) {
- return visitBiFunction(children, builder::greaterThan,
builder::lessThan);
+ return visitComparison(
+ children,
+ negated,
+ builder::lessOrEqual,
+ builder::greaterOrEqual,
+ builder::greaterThan,
+ builder::lessThan);
} else if (func == BuiltInFunctionDefinitions.GREATER_THAN_OR_EQUAL) {
- return visitBiFunction(children, builder::greaterOrEqual,
builder::lessOrEqual);
+ return visitComparison(
+ children,
+ negated,
+ builder::lessThan,
+ builder::greaterThan,
+ builder::greaterOrEqual,
+ builder::lessOrEqual);
} else if (func == BuiltInFunctionDefinitions.LESS_THAN) {
- return visitBiFunction(children, builder::lessThan,
builder::greaterThan);
+ return visitComparison(
+ children,
+ negated,
+ builder::greaterOrEqual,
+ builder::lessOrEqual,
+ builder::lessThan,
+ builder::greaterThan);
} else if (func == BuiltInFunctionDefinitions.LESS_THAN_OR_EQUAL) {
- return visitBiFunction(children, builder::lessOrEqual,
builder::greaterOrEqual);
+ return visitComparison(
+ children,
+ negated,
+ builder::greaterThan,
+ builder::lessThan,
+ builder::lessOrEqual,
+ builder::greaterOrEqual);
} else if (func == BuiltInFunctionDefinitions.IN) {
- FieldReferenceExpression fieldRefExpr =
-
extractFieldReference(children.get(0)).orElseThrow(UnsupportedExpression::new);
+ requireAtLeastArity(children, 2);
+ ResolvedField field = resolveField(children.get(0));
List<Object> literals = new ArrayList<>();
for (int i = 1; i < children.size(); i++) {
- literals.add(extractLiteral(fieldRefExpr.getOutputDataType(),
children.get(i)));
+
literals.add(extractLiteral(field.expression.getOutputDataType(),
children.get(i)));
+ }
+ if (negated) {
+ // SQL WHERE: v NOT IN (..., NULL, ...) is never true. Passing
NULL to
+ // notIn is also unsafe for BSI/range-bitmap file-index
evaluation.
+ if (literals.contains(null)) {
+ return PredicateBuilder.alwaysFalse();
+ }
+ return builder.notIn(field.index, literals);
}
- return builder.in(builder.indexOf(fieldRefExpr.getName()),
literals);
+ return builder.in(field.index, literals);
} else if (func == BuiltInFunctionDefinitions.IS_NULL) {
- return extractFieldReference(children.get(0))
- .map(FieldReferenceExpression::getName)
- .map(builder::indexOf)
- .map(builder::isNull)
- .orElseThrow(UnsupportedExpression::new);
+ requireArity(children, 1);
+ ResolvedField field = resolveField(children.get(0));
+ return negated ? builder.isNotNull(field.index) :
builder.isNull(field.index);
} else if (func == BuiltInFunctionDefinitions.IS_NOT_NULL) {
- return extractFieldReference(children.get(0))
- .map(FieldReferenceExpression::getName)
- .map(builder::indexOf)
- .map(builder::isNotNull)
- .orElseThrow(UnsupportedExpression::new);
+ requireArity(children, 1);
+ ResolvedField field = resolveField(children.get(0));
+ return negated ? builder.isNull(field.index) :
builder.isNotNull(field.index);
} else if (func == BuiltInFunctionDefinitions.BETWEEN) {
- FieldReferenceExpression fieldRefExpr =
-
extractFieldReference(children.get(0)).orElseThrow(UnsupportedExpression::new);
- DataType fieldType = fieldRefExpr.getOutputDataType();
- return builder.between(
- builder.indexOf(fieldRefExpr.getName()),
- extractLiteral(fieldType, children.get(1)),
- extractLiteral(fieldType, children.get(2)));
+ requireArity(children, 3);
+ ResolvedField field = resolveField(children.get(0));
+ DataType fieldType = field.expression.getOutputDataType();
+ Object lower = extractLiteral(fieldType, children.get(1));
+ Object upper = extractLiteral(fieldType, children.get(2));
+ Predicate between = builder.between(field.index, lower, upper);
+ return negated ? negate(between) : between;
Review Comment:
[P1] Preserve TRUE cases for NOT BETWEEN with a NULL bound
Negating a `Between` into Paimon's `NotBetween` leaf is not equivalent when
either bound is NULL: `LeafTernaryFunction.test` immediately returns false for
any null literal. SQL three-valued logic can still make `NOT BETWEEN` true
through the other comparison—for example, `12 NOT BETWEEN 15 AND NULL` is `12 <
15 OR 12 > NULL`, i.e. `TRUE OR UNKNOWN = TRUE`, but the pushed predicate
returns false and drops the row. The symmetric case occurs with a NULL lower
bound and a value above the upper bound. Because this filter is accepted and no
Flink residual remains, please either keep null-bound NOT BETWEEN as a residual
or build the two comparisons with their three-valued truth cases explicitly,
and add both null-bound integration cases.
--
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]
