Stephen0421 commented on code in PR #9427:
URL: https://github.com/apache/paimon/pull/9427#discussion_r3920649696
##########
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:
Fixed as suggested.
Negated `BETWEEN` with a NULL lower or upper bound is now left as a residual
filter. `LeafTernaryFunction.test` returns false if any literal is null, but
`12 NOT BETWEEN 15 AND NULL` is `TRUE OR UNKNOWN` = TRUE, so pushing
`NotBetween` would drop the row.
Non-negated `BETWEEN` with a null bound is unchanged. Covered by
converter/source tests plus an IT for both null-bound cases (`v NOT BETWEEN 15
AND NULL` keeps values below the lower bound; `v NOT BETWEEN NULL AND 10` keeps
values above the upper bound).
##########
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
Review Comment:
Fixed as suggested.
Every newly introduced negated FLOAT/DOUBLE comparison family is now
residual: equality, inequality, `IN`, and `BETWEEN` (in addition to the
previous `>`/`>=`/`<`/`<=` guard). Pushing them through
`Float`/`Double.compareTo` is not equivalent to Flink's Java operators for NaN
identity or signed zeros. Pre-existing non-negated float comparisons are
unchanged.
Simple SQL such as `NOT (d = NaN)` / `d NOT IN (NaN)` / `NOT (d <> 0.0)` is
often normalized by Flink to `<>(d, NaN)` or `=(d, 0.0)` before `applyFilters`,
and `CAST(-0.0 AS DOUBLE)` is `+0.0` in Flink, so those queries never reach
this path. Coverage is unsimplified ASTs in `PredicateConverterTest` and
`FlinkTableSourceTest` (including NaN and both signed zeros).
--
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]
