julianhyde commented on code in PR #4465:
URL: https://github.com/apache/calcite/pull/4465#discussion_r2261791181
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java:
##########
@@ -318,12 +318,19 @@ public RelOptPredicateList getPredicates(Filter filter,
RelMetadataQuery mq) {
final RelNode input = filter.getInput();
final RexBuilder rexBuilder = filter.getCluster().getRexBuilder();
final RelOptPredicateList inputInfo = mq.getPulledUpPredicates(input);
-
+ List<RexNode> predicates =
+
RexUtil.retainDeterministic(RelOptUtil.conjunctions(filter.getCondition()));
+ RelOptCluster cluster = filter.getCluster();
+ final RexExecutor executor =
+ Util.first(cluster.getPlanner().getExecutor(), RexUtil.EXECUTOR);
+ RexSimplify simplify = new RexSimplify(rexBuilder,
RelOptPredicateList.EMPTY, executor);
+ // The RelOptPredicateList data structure requires this invariant: no
comparisons with null
+ List<RexNode> simplified = predicates.stream()
Review Comment:
Invoking `RexSimplify` every time that `getPredicates(Filter)` is called is
likely to be expensive.
Could we make this an invariant in `Filter` (which will cause the
constructor to throw)? Or perhaps add the logic to `RexBuilder.filter`, which
will catch most cases.
Is there a quick way to find comparisons with null that doesn't invoke
`RexSimplify`? (Even if `RexSimplify` is cheap today, I worry that someone will
make it smarter and more expensive in the future.) If so, that could be called
in the `Filter` constructor.
I don't see much point to letting people create `Filter` instances (ditto
`Join` instances) that never evaluate to true, especially if giving people that
privilege makes "sensible" expressions slower for everyone else.
##########
core/src/main/java/org/apache/calcite/plan/RelOptPredicateList.java:
##########
@@ -104,6 +106,36 @@ private RelOptPredicateList(ImmutableList<RexNode>
pulledUpPredicates,
this.rightInferredPredicates =
requireNonNull(rightInferredPredicates, "rightInferredPredicates");
this.constantMap = requireNonNull(constantMap, "constantMap");
+
+ // Validate invariants required
+ // (unfortunately the style rules don't allow us to move this to a
separate function).
+ // Do not allow comparisons with null literals in pulledUpPredicates
+ for (RexNode predicate : this.pulledUpPredicates) {
Review Comment:
If it helps, you could add a comment that the private constructor must
remain private, and must remain the only constructor.
--
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]
