Stephen0421 commented on code in PR #9427:
URL: https://github.com/apache/paimon/pull/9427#discussion_r3893206940
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/PredicateConverter.java:
##########
@@ -74,74 +74,93 @@ 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) {
Review Comment:
Thanks for the catch.
Negated `FLOAT`/`DOUBLE` inequalities (`>`, `>=`, `<`, `<=`) are now left as
residual filters. Rewriting `NOT (NaN > 1.0)` to `LessOrEqual` is not
equivalent: Flink uses Java operators (`NOT (NaN > 1.0)` is true), while Paimon
orders NaN via `compareTo` and would drop the row if the rewrite were pushed.
One note: simple SQL such as `WHERE NOT (d > 1.0)` is often simplified by
Flink to `d <= 1.0` *before* `applyFilters`, so that query never reaches this
path. The guard is for unsimplified `NOT` expressions that still arrive at the
converter (for example De Morgan over `AND`/`OR`). Covered by converter/source
unit tests, including a compound `NOT (float_cmp AND ...)`.
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/PredicateConverter.java:
##########
@@ -74,74 +74,93 @@ 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 negated
+ ? visitBiFunction(children, builder::lessOrEqual,
builder::greaterOrEqual)
+ : visitBiFunction(children, builder::greaterThan,
builder::lessThan);
} else if (func == BuiltInFunctionDefinitions.GREATER_THAN_OR_EQUAL) {
- return visitBiFunction(children, builder::greaterOrEqual,
builder::lessOrEqual);
+ return negated
+ ? visitBiFunction(children, builder::lessThan,
builder::greaterThan)
+ : visitBiFunction(children, builder::greaterOrEqual,
builder::lessOrEqual);
} else if (func == BuiltInFunctionDefinitions.LESS_THAN) {
- return visitBiFunction(children, builder::lessThan,
builder::greaterThan);
+ return negated
+ ? visitBiFunction(children, builder::greaterOrEqual,
builder::lessOrEqual)
+ : visitBiFunction(children, builder::lessThan,
builder::greaterThan);
} else if (func == BuiltInFunctionDefinitions.LESS_THAN_OR_EQUAL) {
- return visitBiFunction(children, builder::lessOrEqual,
builder::greaterOrEqual);
+ return negated
+ ? visitBiFunction(children, builder::greaterThan,
builder::lessThan)
+ : visitBiFunction(children, 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)));
}
- return builder.in(builder.indexOf(fieldRefExpr.getName()),
literals);
+ return negated
+ ? builder.notIn(field.index, literals)
Review Comment:
Fixed as suggested.
When a negated `IN` list contains `NULL`, conversion now returns
`alwaysFalse()`. Under SQL `WHERE` that expression is never true, and passing
`NULL` into `notIn` can NPE in BSI (`valueMapper` returns null, then `value ==
Long.MIN_VALUE` unboxes it) and is also unsafe for range-bitmap.
Added unit tests plus an IT with `file-index.bsi.columns` and
`file-index.range-bitmap.columns` enabled (`WHERE v NOT IN (1, NULL, 3)` →
empty, no crash; `NOT IN (1, 3)` still matches).
--
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]
