[
https://issues.apache.org/jira/browse/CALCITE-2582?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16625527#comment-16625527
]
Stamatis Zampetakis commented on CALCITE-2582:
----------------------------------------------
{quote}I don't mind you splitting the work into separate commits. But I am very
wary of letting people do a simple solution that is adequate for their
purposes. They need to do their part in solving the technical debt.
{quote}
[~julianhyde] I totally understand. I am just saying that things have different
priorities for contributors and trying to solve everything at once just makes
the things slower. I made this fix mostly to allow me to make a clean commit
(and easier review) for a totally different issue (i.e., CALCITE-2554). I am
putting it like this just to illustrate the fact that while this issue (or
another issue) is open, somebody else might lose some time in debugging till he
realizes that there is a bug in another part of the code.
I like the approach with the copying method in the RelBuilder but I don't like
a lot the fact that it involves a callback. It provides a lot of freedom and
users/developers can start doing pretty exotic stuff with it which can be much
more than copying. Maybe a simpler copy method (or RelBuilder#push(RelNode,
boolean) with an additional copy parameter) that dispatches to the appropriate
RelFactories interface based on the type of the node. In any case, I need to
put some more thought into it.
[~vladimirsitnikov]
{quote}That is perfectly fine.
{quote}
It still bothers me a bit the fact that while somebody may expect an
EnumerableFilter he will obtain a LogicalValues (an operator in a different
convention).
{quote}So for now I'm inclined to just adding RexSimplify to the rule, however
it makes sense to make it in line with RelBuilder one
{quote}
I agree but I don't see from where we can obtain the context.
{quote}org.apache.calcite.rel.rules.FilterProjectTransposeRule#INSTANCE is a
generic rule, and it suits all kinds of Filters and Projects thanks to copy
parameters.
{quote}
But apparently, it is not enough otherwise we could make it a singleton.
Moreover, it seems that the Javadoc of the class is no longer correct since we
are not treating exclusively logical operators.
Fun fact: it seems that all calls to the FilterProjectTransposeRule constructor
(from calcite projects) use copyFilter = true and copyProject = true.
{quote}The idea of having EnumerableFilterProjectTransposeRule,
DruidFilterProjectTransposeRule, etc, etc does not sound right.
{quote}
Adding more classes for sure not, but instantiating the rule with a different
RelBuilderFactory does not seem too bad. According to the Javadoc, It seems
that RelBuilderFactory was introduced exactly for this reason.
> FilterProjectTransposeRule does not always simplify the new filter condition
> ----------------------------------------------------------------------------
>
> Key: CALCITE-2582
> URL: https://issues.apache.org/jira/browse/CALCITE-2582
> Project: Calcite
> Issue Type: Improvement
> Components: core
> Affects Versions: 1.17.0
> Reporter: Stamatis Zampetakis
> Assignee: Julian Hyde
> Priority: Minor
> Fix For: 1.18.0
>
>
> After pushing the filter below the project a new condition is going to be
> generated along with a new Filter operator. The new condition is not going to
> be simplified if the filter operator is copied and not created using the
> RelBuilder.
> Thus the resulting plan may contain redundant conditions which can have a
> slight impact on performance. Apart, from that tests verifying the resulting
> (logical/physical) plan may produce indeterministic results if the rule is
> applied with (a different order and in combination with other rules).
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
