Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-1350

Refactor FilterPushDownRule::visitJoin() into well-defined, small methods

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11.0
    • Component/s: Planner/Optimizer
    • Labels:
      None

      Description

      FilterPushDownRule::visitJoin() is too long and complicated. It handles various cases in a single method. We need to refactor this method into several small and well-defined methods.

      1. TAJO-1350_2.patch
        45 kB
        Jihoon Son
      2. TAJO-1350_3.patch
        44 kB
        Jihoon Son
      3. TAJO-1350_4.patch
        46 kB
        Jihoon Son
      4. TAJO-1350.patch
        38 kB
        Jihoon Son

        Issue Links

          Activity

          Hide
          jihoonson Jihoon Son added a comment -

          If others are ok, I'd like to take this issue.

          Show
          jihoonson Jihoon Son added a comment - If others are ok, I'd like to take this issue.
          Hide
          hyunsik Hyunsik Choi added a comment -

          thank you for volunteering!

          Show
          hyunsik Hyunsik Choi added a comment - thank you for volunteering!
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user jihoonson opened a pull request:

          https://github.com/apache/tajo/pull/384

          TAJO-1350: Refactor FilterPushDownRule::visitJoin() into well-defined, small methods

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/jihoonson/tajo-2 TAJO-1350

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tajo/pull/384.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #384


          commit cf960f99212ebd7c631071af7508036f70e0a210
          Author: Jihoon Son <jihoonson@apache.org>
          Date: 2015-02-19T12:57:04Z

          TAJO-1350

          commit 1bf85f60280dcaeb8b14323530bf3c9fd6b9aba6
          Author: Jihoon Son <jihoonson@apache.org>
          Date: 2015-02-20T03:13:22Z

          TAJO-1350

          commit 5a4e55531aa1c797eead8a940138f61d79ccc0be
          Author: Jihoon Son <jihoonson@apache.org>
          Date: 2015-02-20T03:23:39Z

          TAJO-1350

          commit 1bad40b88719da67adf3aa5c103879135f536aee
          Author: Jihoon Son <jihoonson@apache.org>
          Date: 2015-02-20T03:46:15Z

          TAJO-1350

          commit 8b1cd6441ebe6794a3675bd4a64b652af82f4356
          Author: Jihoon Son <jihoonson@apache.org>
          Date: 2015-02-20T03:52:21Z

          TAJO-1350


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user jihoonson opened a pull request: https://github.com/apache/tajo/pull/384 TAJO-1350 : Refactor FilterPushDownRule::visitJoin() into well-defined, small methods You can merge this pull request into a Git repository by running: $ git pull https://github.com/jihoonson/tajo-2 TAJO-1350 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/384.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #384 commit cf960f99212ebd7c631071af7508036f70e0a210 Author: Jihoon Son <jihoonson@apache.org> Date: 2015-02-19T12:57:04Z TAJO-1350 commit 1bf85f60280dcaeb8b14323530bf3c9fd6b9aba6 Author: Jihoon Son <jihoonson@apache.org> Date: 2015-02-20T03:13:22Z TAJO-1350 commit 5a4e55531aa1c797eead8a940138f61d79ccc0be Author: Jihoon Son <jihoonson@apache.org> Date: 2015-02-20T03:23:39Z TAJO-1350 commit 1bad40b88719da67adf3aa5c103879135f536aee Author: Jihoon Son <jihoonson@apache.org> Date: 2015-02-20T03:46:15Z TAJO-1350 commit 8b1cd6441ebe6794a3675bd4a64b652af82f4356 Author: Jihoon Son <jihoonson@apache.org> Date: 2015-02-20T03:52:21Z TAJO-1350
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r25909557

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java —
          @@ -1963,7 +1964,7 @@ public static boolean checkIfBeEvaluatedAtGroupBy(EvalNode evalNode, GroupbyNode
          }

          public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNode, JoinNode node,

          • boolean isTopMostJoin) {
            + boolean isOnPredicate) {
              • End diff –

          ```isOnPredicate``` seems to be a flag to indicate if a predicate is ON clause?

          But, it seems to be inconsistent with its usecases. In ProjectionPushDownRule, TRUE is given to this parameter. But, in the line 1153, this value is true even though they are not placed ON clause.

          Could you let me know your exact intention about this change?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r25909557 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java — @@ -1963,7 +1964,7 @@ public static boolean checkIfBeEvaluatedAtGroupBy(EvalNode evalNode, GroupbyNode } public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNode, JoinNode node, boolean isTopMostJoin) { + boolean isOnPredicate) { End diff – ```isOnPredicate``` seems to be a flag to indicate if a predicate is ON clause? But, it seems to be inconsistent with its usecases. In ProjectionPushDownRule, TRUE is given to this parameter. But, in the line 1153, this value is true even though they are not placed ON clause. Could you let me know your exact intention about this change?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jihoonson commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-77494963

          @hyunsik thanks for your comment.
          That was my misunderstanding. I fixed it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-77494963 @hyunsik thanks for your comment. That was my misunderstanding. I fixed it.
          Hide
          tajoqa Tajo QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12702982/TAJO-1350.patch
          against master revision release-0.9.0-rc0-185-g1617fa9.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

          +1 checkstyle. The patch generated 0 code style errors.

          -1 findbugs. The patch appears to cause Findbugs (version 2.0.3) to fail.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in tajo-core tajo-plan.

          Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/598//testReport/
          Findbugs results: https://builds.apache.org/job/PreCommit-TAJO-Build/598//findbugsResult
          Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/598//console

          This message is automatically generated.

          Show
          tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12702982/TAJO-1350.patch against master revision release-0.9.0-rc0-185-g1617fa9. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. -1 findbugs. The patch appears to cause Findbugs (version 2.0.3) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in tajo-core tajo-plan. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/598//testReport/ Findbugs results: https://builds.apache.org/job/PreCommit-TAJO-Build/598//findbugsResult Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/598//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r26089560

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java —
          @@ -1978,26 +1979,18 @@ public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNo
          return false;
          }

          • // When a 'case-when' is used with outer join, the case-when expression must be evaluated
          • // at the topmost join operator.
          • // TODO - It's also valid that case-when is evalauted at the topmost outer operator.
          • // But, how can we know there is no further outer join operator after this node?
          • if (containsOuterJoin(block)) {
          • if (!isTopMostJoin) {
          • Collection<EvalNode> found = EvalTreeUtil.findOuterJoinSensitiveEvals(evalNode);
          • if (found.size() > 0) { - return false; - }
          • }
            + /*
            + * For outer joins, only predicates which are specified at the on clause can be evaluated during processing join.
            + * Other predicates from the where clause must be evaluated after the join.
            + * The below code will be modified after improving join operators to keep join filters by themselves (TAJO-1310).
            + */
            + if (PlannerUtil.isOuterJoin(node.getJoinType()) && !isOnPredicate) {
              • End diff –

          Could explain your intension about this change? It would be great if I can hear what the problem (or motivation) is and what your solution is.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r26089560 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java — @@ -1978,26 +1979,18 @@ public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNo return false; } // When a 'case-when' is used with outer join, the case-when expression must be evaluated // at the topmost join operator. // TODO - It's also valid that case-when is evalauted at the topmost outer operator. // But, how can we know there is no further outer join operator after this node? if (containsOuterJoin(block)) { if (!isTopMostJoin) { Collection<EvalNode> found = EvalTreeUtil.findOuterJoinSensitiveEvals(evalNode); if (found.size() > 0) { - return false; - } } + /* + * For outer joins, only predicates which are specified at the on clause can be evaluated during processing join. + * Other predicates from the where clause must be evaluated after the join. + * The below code will be modified after improving join operators to keep join filters by themselves ( TAJO-1310 ). + */ + if (PlannerUtil.isOuterJoin(node.getJoinType()) && !isOnPredicate) { End diff – Could explain your intension about this change? It would be great if I can hear what the problem (or motivation) is and what your solution is.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jihoonson commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r26092914

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java —
          @@ -1978,26 +1979,18 @@ public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNo
          return false;
          }

          • // When a 'case-when' is used with outer join, the case-when expression must be evaluated
          • // at the topmost join operator.
          • // TODO - It's also valid that case-when is evalauted at the topmost outer operator.
          • // But, how can we know there is no further outer join operator after this node?
          • if (containsOuterJoin(block)) {
          • if (!isTopMostJoin) {
          • Collection<EvalNode> found = EvalTreeUtil.findOuterJoinSensitiveEvals(evalNode);
          • if (found.size() > 0) { - return false; - }
          • }
            + /*
            + * For outer joins, only predicates which are specified at the on clause can be evaluated during processing join.
            + * Other predicates from the where clause must be evaluated after the join.
            + * The below code will be modified after improving join operators to keep join filters by themselves (TAJO-1310).
            + */
            + if (PlannerUtil.isOuterJoin(node.getJoinType()) && !isOnPredicate) {
              • End diff –

          Predicates must be checked at every join node rather than the topmost one. Thus, I removed the topMostJoin flag.

          The newly added line is for outer joins. As you know, in the case of the outer joins, only the predicates from the on clauses can be the join conditions. Other predicates from the where clauses are join filters.
          Currently, we maintain join filters in separate selection nodes rather than the join nodes themselves. So, I prevent the filters being pushed down to outer join nodes. This will be improved when TAJO-1310 is resolved.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r26092914 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java — @@ -1978,26 +1979,18 @@ public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNo return false; } // When a 'case-when' is used with outer join, the case-when expression must be evaluated // at the topmost join operator. // TODO - It's also valid that case-when is evalauted at the topmost outer operator. // But, how can we know there is no further outer join operator after this node? if (containsOuterJoin(block)) { if (!isTopMostJoin) { Collection<EvalNode> found = EvalTreeUtil.findOuterJoinSensitiveEvals(evalNode); if (found.size() > 0) { - return false; - } } + /* + * For outer joins, only predicates which are specified at the on clause can be evaluated during processing join. + * Other predicates from the where clause must be evaluated after the join. + * The below code will be modified after improving join operators to keep join filters by themselves ( TAJO-1310 ). + */ + if (PlannerUtil.isOuterJoin(node.getJoinType()) && !isOnPredicate) { End diff – Predicates must be checked at every join node rather than the topmost one. Thus, I removed the topMostJoin flag. The newly added line is for outer joins. As you know, in the case of the outer joins, only the predicates from the on clauses can be the join conditions. Other predicates from the where clauses are join filters. Currently, we maintain join filters in separate selection nodes rather than the join nodes themselves. So, I prevent the filters being pushed down to outer join nodes. This will be improved when TAJO-1310 is resolved.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r26447163

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java —
          @@ -1978,26 +1979,18 @@ public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNo
          return false;
          }

          • // When a 'case-when' is used with outer join, the case-when expression must be evaluated
          • // at the topmost join operator.
          • // TODO - It's also valid that case-when is evalauted at the topmost outer operator.
          • // But, how can we know there is no further outer join operator after this node?
          • if (containsOuterJoin(block)) {
          • if (!isTopMostJoin) {
          • Collection<EvalNode> found = EvalTreeUtil.findOuterJoinSensitiveEvals(evalNode);
          • if (found.size() > 0) { - return false; - }
          • }
            + /*
            + * For outer joins, only predicates which are specified at the on clause can be evaluated during processing join.
            + * Other predicates from the where clause must be evaluated after the join.
            + * The below code will be modified after improving join operators to keep join filters by themselves (TAJO-1310).
            + */
            + if (PlannerUtil.isOuterJoin(node.getJoinType()) && !isOnPredicate) {
              • End diff –

          I'm concerning that this change does not consider the original purpose. Please look over TAJO-428. This change is for workaround of TAJO-428 bug.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r26447163 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java — @@ -1978,26 +1979,18 @@ public static boolean checkIfBeEvaluatedAtJoin(QueryBlock block, EvalNode evalNo return false; } // When a 'case-when' is used with outer join, the case-when expression must be evaluated // at the topmost join operator. // TODO - It's also valid that case-when is evalauted at the topmost outer operator. // But, how can we know there is no further outer join operator after this node? if (containsOuterJoin(block)) { if (!isTopMostJoin) { Collection<EvalNode> found = EvalTreeUtil.findOuterJoinSensitiveEvals(evalNode); if (found.size() > 0) { - return false; - } } + /* + * For outer joins, only predicates which are specified at the on clause can be evaluated during processing join. + * Other predicates from the where clause must be evaluated after the join. + * The below code will be modified after improving join operators to keep join filters by themselves ( TAJO-1310 ). + */ + if (PlannerUtil.isOuterJoin(node.getJoinType()) && !isOnPredicate) { End diff – I'm concerning that this change does not consider the original purpose. Please look over TAJO-428 . This change is for workaround of TAJO-428 bug.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jihoonson commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-81262357

          @hyunsik thanks for your comment.
          I've fixed it. Please review the patch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-81262357 @hyunsik thanks for your comment. I've fixed it. Please review the patch.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r26644814

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java —
          @@ -355,20 +230,144 @@ public LogicalNode visitJoin(FilterPushDownContext context, LogicalPlan plan, Lo
          if (joinNode.getJoinType() == JoinType.CROSS)

          { joinNode.setJoinType(JoinType.INNER); }
          • context.pushingDownFilters.removeAll(matched);
            }
          • context.pushingDownFilters.addAll(outerJoinFilterEvalsExcludePredication);
          • context.pushingDownFilters.addAll(thetaJoinFilter);
            + context.pushingDownFilters.removeAll(matched);
            return joinNode;
            }

          + private static Set<EvalNode> extractNonPushableJoinQuals(final LogicalPlan plan,
          + final LogicalPlan.QueryBlock block,
          + final JoinNode joinNode,
          + final Set<EvalNode> onPredicates,
          + final Set<EvalNode> wherePredicates)
          + throws PlanningException {
          + Set<EvalNode> nonPushableQuals = TUtil.newHashSet();
          + // TODO: non-equi theta join quals must not be pushed until TAJO-742 is resolved.
          + nonPushableQuals.addAll(extractNonEquiThetaJoinQuals(wherePredicates, block, joinNode));
          +
          + // for outer joins
          + if (PlannerUtil.isOuterJoin(joinNode.getJoinType()))

          { + nonPushableQuals.addAll(extractNonPushableOuterJoinQuals(plan, onPredicates, wherePredicates, joinNode)); + }

          + return nonPushableQuals;
          + }
          +
          + /**
          + * For outer joins, pushable predicates can be decided based on their locations in the SQL and types of referencing relations.
          — End diff –

          It seems to exceed 120 columns.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r26644814 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java — @@ -355,20 +230,144 @@ public LogicalNode visitJoin(FilterPushDownContext context, LogicalPlan plan, Lo if (joinNode.getJoinType() == JoinType.CROSS) { joinNode.setJoinType(JoinType.INNER); } context.pushingDownFilters.removeAll(matched); } context.pushingDownFilters.addAll(outerJoinFilterEvalsExcludePredication); context.pushingDownFilters.addAll(thetaJoinFilter); + context.pushingDownFilters.removeAll(matched); return joinNode; } + private static Set<EvalNode> extractNonPushableJoinQuals(final LogicalPlan plan, + final LogicalPlan.QueryBlock block, + final JoinNode joinNode, + final Set<EvalNode> onPredicates, + final Set<EvalNode> wherePredicates) + throws PlanningException { + Set<EvalNode> nonPushableQuals = TUtil.newHashSet(); + // TODO: non-equi theta join quals must not be pushed until TAJO-742 is resolved. + nonPushableQuals.addAll(extractNonEquiThetaJoinQuals(wherePredicates, block, joinNode)); + + // for outer joins + if (PlannerUtil.isOuterJoin(joinNode.getJoinType())) { + nonPushableQuals.addAll(extractNonPushableOuterJoinQuals(plan, onPredicates, wherePredicates, joinNode)); + } + return nonPushableQuals; + } + + /** + * For outer joins, pushable predicates can be decided based on their locations in the SQL and types of referencing relations. — End diff – It seems to exceed 120 columns.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r26644864

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java —
          @@ -355,20 +230,144 @@ public LogicalNode visitJoin(FilterPushDownContext context, LogicalPlan plan, Lo
          if (joinNode.getJoinType() == JoinType.CROSS)

          { joinNode.setJoinType(JoinType.INNER); }
          • context.pushingDownFilters.removeAll(matched);
            }
          • context.pushingDownFilters.addAll(outerJoinFilterEvalsExcludePredication);
          • context.pushingDownFilters.addAll(thetaJoinFilter);
            + context.pushingDownFilters.removeAll(matched);
            return joinNode;
            }

          + private static Set<EvalNode> extractNonPushableJoinQuals(final LogicalPlan plan,
          + final LogicalPlan.QueryBlock block,
          + final JoinNode joinNode,
          + final Set<EvalNode> onPredicates,
          + final Set<EvalNode> wherePredicates)
          + throws PlanningException {
          + Set<EvalNode> nonPushableQuals = TUtil.newHashSet();
          + // TODO: non-equi theta join quals must not be pushed until TAJO-742 is resolved.
          + nonPushableQuals.addAll(extractNonEquiThetaJoinQuals(wherePredicates, block, joinNode));
          +
          + // for outer joins
          + if (PlannerUtil.isOuterJoin(joinNode.getJoinType()))

          { + nonPushableQuals.addAll(extractNonPushableOuterJoinQuals(plan, onPredicates, wherePredicates, joinNode)); + }

          + return nonPushableQuals;
          + }
          +
          + /**
          + * For outer joins, pushable predicates can be decided based on their locations in the SQL and types of referencing relations.
          + *
          + * <h3>Table types</h3>
          + * <ul>
          + * <li>Preserved Row table : The table in an Outer Join that must return all rows.
          + * For left outer joins this is the Left table, for right outer joins it is the Right table.
          + * For full outer joins both tables are Preserved Row tables.</li>
          + * <li>Null Supplying table : This is the table that has nulls filled in for its columns in unmatched rows.
          + * In the non-full outer join case, this is the other table in the Join.
          — End diff –

          Could you check this comment?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r26644864 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java — @@ -355,20 +230,144 @@ public LogicalNode visitJoin(FilterPushDownContext context, LogicalPlan plan, Lo if (joinNode.getJoinType() == JoinType.CROSS) { joinNode.setJoinType(JoinType.INNER); } context.pushingDownFilters.removeAll(matched); } context.pushingDownFilters.addAll(outerJoinFilterEvalsExcludePredication); context.pushingDownFilters.addAll(thetaJoinFilter); + context.pushingDownFilters.removeAll(matched); return joinNode; } + private static Set<EvalNode> extractNonPushableJoinQuals(final LogicalPlan plan, + final LogicalPlan.QueryBlock block, + final JoinNode joinNode, + final Set<EvalNode> onPredicates, + final Set<EvalNode> wherePredicates) + throws PlanningException { + Set<EvalNode> nonPushableQuals = TUtil.newHashSet(); + // TODO: non-equi theta join quals must not be pushed until TAJO-742 is resolved. + nonPushableQuals.addAll(extractNonEquiThetaJoinQuals(wherePredicates, block, joinNode)); + + // for outer joins + if (PlannerUtil.isOuterJoin(joinNode.getJoinType())) { + nonPushableQuals.addAll(extractNonPushableOuterJoinQuals(plan, onPredicates, wherePredicates, joinNode)); + } + return nonPushableQuals; + } + + /** + * For outer joins, pushable predicates can be decided based on their locations in the SQL and types of referencing relations. + * + * <h3>Table types</h3> + * <ul> + * <li>Preserved Row table : The table in an Outer Join that must return all rows. + * For left outer joins this is the Left table, for right outer joins it is the Right table. + * For full outer joins both tables are Preserved Row tables.</li> + * <li>Null Supplying table : This is the table that has nulls filled in for its columns in unmatched rows. + * In the non-full outer join case, this is the other table in the Join. — End diff – Could you check this comment?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r26645305

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java —
          @@ -355,20 +230,144 @@ public LogicalNode visitJoin(FilterPushDownContext context, LogicalPlan plan, Lo
          if (joinNode.getJoinType() == JoinType.CROSS)

          { joinNode.setJoinType(JoinType.INNER); }
          • context.pushingDownFilters.removeAll(matched);
            }
          • context.pushingDownFilters.addAll(outerJoinFilterEvalsExcludePredication);
          • context.pushingDownFilters.addAll(thetaJoinFilter);
            + context.pushingDownFilters.removeAll(matched);
            return joinNode;
            }

          + private static Set<EvalNode> extractNonPushableJoinQuals(final LogicalPlan plan,
          + final LogicalPlan.QueryBlock block,
          + final JoinNode joinNode,
          + final Set<EvalNode> onPredicates,
          + final Set<EvalNode> wherePredicates)
          + throws PlanningException {
          + Set<EvalNode> nonPushableQuals = TUtil.newHashSet();
          + // TODO: non-equi theta join quals must not be pushed until TAJO-742 is resolved.
          + nonPushableQuals.addAll(extractNonEquiThetaJoinQuals(wherePredicates, block, joinNode));
          +
          + // for outer joins
          + if (PlannerUtil.isOuterJoin(joinNode.getJoinType()))

          { + nonPushableQuals.addAll(extractNonPushableOuterJoinQuals(plan, onPredicates, wherePredicates, joinNode)); + }

          + return nonPushableQuals;
          + }
          +
          + /**
          + * For outer joins, pushable predicates can be decided based on their locations in the SQL and types of referencing relations.
          + *
          + * <h3>Table types</h3>
          + * <ul>
          + * <li>Preserved Row table : The table in an Outer Join that must return all rows.
          + * For left outer joins this is the Left table, for right outer joins it is the Right table.
          + * For full outer joins both tables are Preserved Row tables.</li>
          + * <li>Null Supplying table : This is the table that has nulls filled in for its columns in unmatched rows.
          + * In the non-full outer join case, this is the other table in the Join.
          + * For full outer joins both tables are also Null Supplying tables.</li>
          + * </ul>
          + *
          + * <h3>Predicate types</h3>
          + * <ul>
          + * <li>During Join predicate : A predicate that is in the JOIN ON clause.</li>
          + * <li>After Join predicate : A predicate that is in the WHERE clause.</li>
          + * </ul>
          + *
          + * <h3>Predicate Pushdown Rules</h3>
          + * <ol>
          + * <li>During Join predicates cannot be pushed past Preserved Row tables.</li>
          + * <li>After Join predicates cannot be pushed past Null Supplying tables.</li>
          + * </ol>
          + */
          + private static Set<EvalNode> extractNonPushableOuterJoinQuals(final LogicalPlan plan,
          + final Set<EvalNode> onPredicates,
          + final Set<EvalNode> wherePredicates,
          + final JoinNode joinNode) throws PlanningException {
          + Set<String> nullSupplyingTableNameSet = TUtil.newHashSet();
          — End diff –

          This method seems to be changed to be even more intuitive.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r26645305 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java — @@ -355,20 +230,144 @@ public LogicalNode visitJoin(FilterPushDownContext context, LogicalPlan plan, Lo if (joinNode.getJoinType() == JoinType.CROSS) { joinNode.setJoinType(JoinType.INNER); } context.pushingDownFilters.removeAll(matched); } context.pushingDownFilters.addAll(outerJoinFilterEvalsExcludePredication); context.pushingDownFilters.addAll(thetaJoinFilter); + context.pushingDownFilters.removeAll(matched); return joinNode; } + private static Set<EvalNode> extractNonPushableJoinQuals(final LogicalPlan plan, + final LogicalPlan.QueryBlock block, + final JoinNode joinNode, + final Set<EvalNode> onPredicates, + final Set<EvalNode> wherePredicates) + throws PlanningException { + Set<EvalNode> nonPushableQuals = TUtil.newHashSet(); + // TODO: non-equi theta join quals must not be pushed until TAJO-742 is resolved. + nonPushableQuals.addAll(extractNonEquiThetaJoinQuals(wherePredicates, block, joinNode)); + + // for outer joins + if (PlannerUtil.isOuterJoin(joinNode.getJoinType())) { + nonPushableQuals.addAll(extractNonPushableOuterJoinQuals(plan, onPredicates, wherePredicates, joinNode)); + } + return nonPushableQuals; + } + + /** + * For outer joins, pushable predicates can be decided based on their locations in the SQL and types of referencing relations. + * + * <h3>Table types</h3> + * <ul> + * <li>Preserved Row table : The table in an Outer Join that must return all rows. + * For left outer joins this is the Left table, for right outer joins it is the Right table. + * For full outer joins both tables are Preserved Row tables.</li> + * <li>Null Supplying table : This is the table that has nulls filled in for its columns in unmatched rows. + * In the non-full outer join case, this is the other table in the Join. + * For full outer joins both tables are also Null Supplying tables.</li> + * </ul> + * + * <h3>Predicate types</h3> + * <ul> + * <li>During Join predicate : A predicate that is in the JOIN ON clause.</li> + * <li>After Join predicate : A predicate that is in the WHERE clause.</li> + * </ul> + * + * <h3>Predicate Pushdown Rules</h3> + * <ol> + * <li>During Join predicates cannot be pushed past Preserved Row tables.</li> + * <li>After Join predicates cannot be pushed past Null Supplying tables.</li> + * </ol> + */ + private static Set<EvalNode> extractNonPushableOuterJoinQuals(final LogicalPlan plan, + final Set<EvalNode> onPredicates, + final Set<EvalNode> wherePredicates, + final JoinNode joinNode) throws PlanningException { + Set<String> nullSupplyingTableNameSet = TUtil.newHashSet(); — End diff – This method seems to be changed to be even more intuitive.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-82821794

          I leave some trivial comments on some java docs. Others are all good. Especially, extractNonPushableOuterJoinQuals looks intuitive.

          After you reflect my comment, I'll finish the review.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-82821794 I leave some trivial comments on some java docs. Others are all good. Especially, extractNonPushableOuterJoinQuals looks intuitive. After you reflect my comment, I'll finish the review.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jihoonson commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-82950907

          Thanks. I improved java docs.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-82950907 Thanks. I improved java docs.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-83848989

          Could you trigger CI again?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-83848989 Could you trigger CI again?
          Hide
          jihoonson Jihoon Son added a comment -

          Rebased.

          Show
          jihoonson Jihoon Son added a comment - Rebased.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jihoonson commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-84762423

          Rebased.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-84762423 Rebased.
          Hide
          jihoonson Jihoon Son added a comment -

          This patch fixes the bug reported at TAJO-1445.
          I've added a test for that bug.

          Show
          jihoonson Jihoon Son added a comment - This patch fixes the bug reported at TAJO-1445 . I've added a test for that bug.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86348625

          @jihoonson

          Thanks for your contribution.
          But I failed to run an unit test for TestNetTypes as following:

          ```
          -------------------------------------------------------------------------------
          Test set: org.apache.tajo.engine.query.TestNetTypes
          -------------------------------------------------------------------------------
          Tests run: 6, Failures: 6, Errors: 0, Skipped: 0, Time elapsed: 0.012 sec <<< FAILURE! - in org.apache.tajo.engine.query.TestNetTypes
          testSort2(org.apache.tajo.engine.query.TestNetTypes) Time elapsed: 0 sec <<< FAILURE!
          java.lang.AssertionError: file:/Users/blrunner/Source/tajo/tajo-core/target/test-classes/dataset/TestNetTypes/table1 existence check
          at org.junit.Assert.fail(Assert.java:88)
          at org.junit.Assert.assertTrue(Assert.java:41)
          at org.apache.tajo.QueryTestCaseBase.getDataSetFile(QueryTestCaseBase.java:605)
          at org.apache.tajo.QueryTestCaseBase.executeDDL(QueryTestCaseBase.java:656)
          at org.apache.tajo.QueryTestCaseBase.executeDDL(QueryTestCaseBase.java:643)
          at org.apache.tajo.engine.query.TestNetTypes.setUp(TestNetTypes.java:32)
          ```

          The strange thing is that it only is reproduced on console. I found that it had run successfully with IDE.
          Could you check this problem on your development environment?

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86348625 @jihoonson Thanks for your contribution. But I failed to run an unit test for TestNetTypes as following: ``` ------------------------------------------------------------------------------- Test set: org.apache.tajo.engine.query.TestNetTypes ------------------------------------------------------------------------------- Tests run: 6, Failures: 6, Errors: 0, Skipped: 0, Time elapsed: 0.012 sec <<< FAILURE! - in org.apache.tajo.engine.query.TestNetTypes testSort2(org.apache.tajo.engine.query.TestNetTypes) Time elapsed: 0 sec <<< FAILURE! java.lang.AssertionError: file:/Users/blrunner/Source/tajo/tajo-core/target/test-classes/dataset/TestNetTypes/table1 existence check at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.assertTrue(Assert.java:41) at org.apache.tajo.QueryTestCaseBase.getDataSetFile(QueryTestCaseBase.java:605) at org.apache.tajo.QueryTestCaseBase.executeDDL(QueryTestCaseBase.java:656) at org.apache.tajo.QueryTestCaseBase.executeDDL(QueryTestCaseBase.java:643) at org.apache.tajo.engine.query.TestNetTypes.setUp(TestNetTypes.java:32) ``` The strange thing is that it only is reproduced on console. I found that it had run successfully with IDE. Could you check this problem on your development environment?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86359111

          @jihoonson

          It seems that there was some problems on my laptop. I can't reproduce it again.
          And I'm still reviewing. I'll finish soon.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86359111 @jihoonson It seems that there was some problems on my laptop. I can't reproduce it again. And I'm still reviewing. I'll finish soon.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jihoonson commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86361068

          Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86361068 Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/384#discussion_r27196435

          — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java —
          @@ -1972,6 +1976,52 @@ public static boolean checkIfBeEvaluatedAtGroupBy(EvalNode evalNode, GroupbyNode
          return true;
          }

          + public static boolean isEvaluatableJoinQual(QueryBlock block, EvalNode evalNode, JoinNode node,
          + boolean isOnPredicate, boolean isTopMostJoin) {
          +
          + if (checkIfBeEvaluatedAtJoin(block, evalNode, node, isTopMostJoin)) {
          +
          + if (isNonEquiThetaJoinQual(block, node, evalNode))

          { + return false; + }

          +
          + if (PlannerUtil.isOuterJoin(node.getJoinType())) {
          + /*
          — End diff –

          It seems to exceed 120 columns.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/384#discussion_r27196435 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java — @@ -1972,6 +1976,52 @@ public static boolean checkIfBeEvaluatedAtGroupBy(EvalNode evalNode, GroupbyNode return true; } + public static boolean isEvaluatableJoinQual(QueryBlock block, EvalNode evalNode, JoinNode node, + boolean isOnPredicate, boolean isTopMostJoin) { + + if (checkIfBeEvaluatedAtJoin(block, evalNode, node, isTopMostJoin)) { + + if (isNonEquiThetaJoinQual(block, node, evalNode)) { + return false; + } + + if (PlannerUtil.isOuterJoin(node.getJoinType())) { + /* — End diff – It seems to exceed 120 columns.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86415309

          +1

          Thanks for your efforts.
          Your works will be helpful to other contributors.

          And I leaved a trivial comment. Before you push it, you need to check 120 columns at LogicalPlanner::isEvaluatableJoinQual.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86415309 +1 Thanks for your efforts. Your works will be helpful to other contributors. And I leaved a trivial comment. Before you push it, you need to check 120 columns at LogicalPlanner::isEvaluatableJoinQual.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86696028

          +1

          LGTM!

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86696028 +1 LGTM!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sirpkt commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86769740

          +1

          By the way, during the test, I found that there still exists some more room to improve push filter down.

              select
                n1.n_nationkey,
                n1.n_name,
                n2.n_name
              from nation n1, nation n2, nation n3 where (n1.n_nationkey in (1,2) or n2.n_nationkey in (2)) and n2.n_nationkey = n3.n_nationkey and n1.n_nationkey = n2.n_nationkey
              order by n1.n_nationkey;
              

          For above query, resulting optimized query plan is

              PROJECTION(5)
                => Targets: default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT)
                => out schema: {(3) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT)}
                => in  schema: {(4) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)}
                 SORT(4)
                   => Sort Keys: default.n1.n_nationkey (INT4) (asc)
                    SELECTION(3)
                      => Search Cond: default.n1.n_nationkey (INT4) IN (1, 2) OR default.n2.n_nationkey (INT4) IN (2)
                       JOIN(10)(INNER)
                         => Join Cond: default.n2.n_nationkey (INT4) = default.n3.n_nationkey (INT4)
                         => target list: default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)
                         => out schema: {(4) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)}
                         => in schema: {(5) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT), default.n2.n_nationkey (INT4), default.n3.n_nationkey (INT4)}
                          SCAN(2) on default.nation as n3
                            => target list: default.n3.n_nationkey (INT4)
                            => out schema: {(1) default.n3.n_nationkey (INT4)}
                            => in schema: {(4) default.n3.n_nationkey (INT4), default.n3.n_name (TEXT), default.n3.n_regionkey (INT4), default.n3.n_comment (TEXT)}
                          JOIN(9)(INNER)
                            => Join Cond: default.n1.n_nationkey (INT4) = default.n2.n_nationkey (INT4)
                            => target list: default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)
                            => out schema: {(4) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)}
                            => in schema: {(4) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)}
                             SCAN(1) on default.nation as n2
                               => target list: default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)
                               => out schema: {(2) default.n2.n_name (TEXT), default.n2.n_nationkey (INT4)}
                               => in schema: {(4) default.n2.n_nationkey (INT4), default.n2.n_name (TEXT), default.n2.n_regionkey (INT4), default.n2.n_comment (TEXT)}
                             SCAN(0) on default.nation as n1
                               => target list: default.n1.n_nationkey (INT4), default.n1.n_name (TEXT)
                               => out schema: {(2) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT)}
                               => in schema: {(4) default.n1.n_nationkey (INT4), default.n1.n_name (TEXT), default.n1.n_regionkey (INT4), default.n1.n_comment (TEXT)}
              

          It would be more nice if we can do SELECTION(3) just after JOIN(9).
          I think, After TAJO-1310 is resolved, we can further enhance filter push down with new join filter feature.

          Show
          githubbot ASF GitHub Bot added a comment - Github user sirpkt commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86769740 +1 By the way, during the test, I found that there still exists some more room to improve push filter down. select n1.n_nationkey, n1.n_name, n2.n_name from nation n1, nation n2, nation n3 where (n1.n_nationkey in (1,2) or n2.n_nationkey in (2)) and n2.n_nationkey = n3.n_nationkey and n1.n_nationkey = n2.n_nationkey order by n1.n_nationkey; For above query, resulting optimized query plan is PROJECTION(5) => Targets: default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT) => out schema: {(3) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT)} => in schema: {(4) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT), default .n2.n_nationkey (INT4)} SORT(4) => Sort Keys: default .n1.n_nationkey (INT4) (asc) SELECTION(3) => Search Cond: default .n1.n_nationkey (INT4) IN (1, 2) OR default .n2.n_nationkey (INT4) IN (2) JOIN(10)(INNER) => Join Cond: default .n2.n_nationkey (INT4) = default .n3.n_nationkey (INT4) => target list: default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT), default .n2.n_nationkey (INT4) => out schema: {(4) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT), default .n2.n_nationkey (INT4)} => in schema: {(5) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT), default .n2.n_nationkey (INT4), default .n3.n_nationkey (INT4)} SCAN(2) on default .nation as n3 => target list: default .n3.n_nationkey (INT4) => out schema: {(1) default .n3.n_nationkey (INT4)} => in schema: {(4) default .n3.n_nationkey (INT4), default .n3.n_name (TEXT), default .n3.n_regionkey (INT4), default .n3.n_comment (TEXT)} JOIN(9)(INNER) => Join Cond: default .n1.n_nationkey (INT4) = default .n2.n_nationkey (INT4) => target list: default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT), default .n2.n_nationkey (INT4) => out schema: {(4) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT), default .n2.n_nationkey (INT4)} => in schema: {(4) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n2.n_name (TEXT), default .n2.n_nationkey (INT4)} SCAN(1) on default .nation as n2 => target list: default .n2.n_name (TEXT), default .n2.n_nationkey (INT4) => out schema: {(2) default .n2.n_name (TEXT), default .n2.n_nationkey (INT4)} => in schema: {(4) default .n2.n_nationkey (INT4), default .n2.n_name (TEXT), default .n2.n_regionkey (INT4), default .n2.n_comment (TEXT)} SCAN(0) on default .nation as n1 => target list: default .n1.n_nationkey (INT4), default .n1.n_name (TEXT) => out schema: {(2) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT)} => in schema: {(4) default .n1.n_nationkey (INT4), default .n1.n_name (TEXT), default .n1.n_regionkey (INT4), default .n1.n_comment (TEXT)} It would be more nice if we can do SELECTION(3) just after JOIN(9). I think, After TAJO-1310 is resolved, we can further enhance filter push down with new join filter feature.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sirpkt commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86771989

          Oh above example may not be suitable.
          Forget it

          Show
          githubbot ASF GitHub Bot added a comment - Github user sirpkt commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86771989 Oh above example may not be suitable. Forget it
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jihoonson commented on the pull request:

          https://github.com/apache/tajo/pull/384#issuecomment-86791563

          Thanks for all your valuable reviews. I'll commit shortly.

          @sirpkt, I also think that the selection should be pushed down to after the first join. This will improve the query performance.
          To resolve this problem, I think that we need to add another filter pushdown optimization phase after join ordering. Please note that the filter pushdown phase is currently called only before join ordering.
          After join ordering, the best position of each predicate will be differernt. So, it seems the problem of our query optimization rather than just a problem of filter pushdown. I'll create another issue.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/384#issuecomment-86791563 Thanks for all your valuable reviews. I'll commit shortly. @sirpkt, I also think that the selection should be pushed down to after the first join. This will improve the query performance. To resolve this problem, I think that we need to add another filter pushdown optimization phase after join ordering. Please note that the filter pushdown phase is currently called only before join ordering. After join ordering, the best position of each predicate will be differernt. So, it seems the problem of our query optimization rather than just a problem of filter pushdown. I'll create another issue.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tajo/pull/384

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/384
          Hide
          jihoonson Jihoon Son added a comment -

          Committed to master.

          Show
          jihoonson Jihoon Son added a comment - Committed to master.
          Hide
          hudson Hudson added a comment -

          ABORTED: Integrated in Tajo-master-CODEGEN-build #271 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/271/)
          TAJO-1350: Refactor FilterPushDownRule::visitJoin() into well-defined, small methods. (jihoon) (jihoonson: rev b1e174eec4c15142b6fa518b6803579bb0788e8e)

          • tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen2.sql
          • CHANGES
          • tajo-core/src/test/resources/queries/TestJoinQuery/testCrossJoinAndCaseWhen.sql
          • tajo-core/src/test/java/org/apache/tajo/engine/query/TestJoinQuery.java
          • tajo-core/src/test/resources/results/TestJoinQuery/testJoinWithOrPredicates.result
          • tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java
          • tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen2.result
          • tajo-core/src/test/resources/queries/TestJoinQuery/testJoinWithOrPredicates.sql
          • tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen.result
          • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java
          • tajo-plan/src/main/java/org/apache/tajo/plan/util/PlannerUtil.java
          • tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/ProjectionPushDownRule.java
          • tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java
          • tajo-core/src/test/resources/results/TestJoinQuery/testCrossJoinAndCaseWhen.result
          • tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen.sql
          • tajo-core/src/test/resources/queries/TestJoinQuery/testInnerJoinAndCaseWhen.sql
          • tajo-core/src/test/resources/results/TestJoinQuery/testInnerJoinAndCaseWhen.result
          Show
          hudson Hudson added a comment - ABORTED: Integrated in Tajo-master-CODEGEN-build #271 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/271/ ) TAJO-1350 : Refactor FilterPushDownRule::visitJoin() into well-defined, small methods. (jihoon) (jihoonson: rev b1e174eec4c15142b6fa518b6803579bb0788e8e) tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen2.sql CHANGES tajo-core/src/test/resources/queries/TestJoinQuery/testCrossJoinAndCaseWhen.sql tajo-core/src/test/java/org/apache/tajo/engine/query/TestJoinQuery.java tajo-core/src/test/resources/results/TestJoinQuery/testJoinWithOrPredicates.result tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen2.result tajo-core/src/test/resources/queries/TestJoinQuery/testJoinWithOrPredicates.sql tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen.result tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java tajo-plan/src/main/java/org/apache/tajo/plan/util/PlannerUtil.java tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/ProjectionPushDownRule.java tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java tajo-core/src/test/resources/results/TestJoinQuery/testCrossJoinAndCaseWhen.result tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen.sql tajo-core/src/test/resources/queries/TestJoinQuery/testInnerJoinAndCaseWhen.sql tajo-core/src/test/resources/results/TestJoinQuery/testInnerJoinAndCaseWhen.result
          Hide
          hudson Hudson added a comment -

          ABORTED: Integrated in Tajo-master-build #633 (See https://builds.apache.org/job/Tajo-master-build/633/)
          TAJO-1350: Refactor FilterPushDownRule::visitJoin() into well-defined, small methods. (jihoon) (jihoonson: rev b1e174eec4c15142b6fa518b6803579bb0788e8e)

          • tajo-core/src/test/resources/results/TestJoinQuery/testInnerJoinAndCaseWhen.result
          • tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen2.result
          • tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java
          • tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen.result
          • tajo-core/src/test/resources/results/TestJoinQuery/testJoinWithOrPredicates.result
          • tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java
          • tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen.sql
          • tajo-plan/src/main/java/org/apache/tajo/plan/util/PlannerUtil.java
          • tajo-core/src/test/resources/queries/TestJoinQuery/testInnerJoinAndCaseWhen.sql
          • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java
          • tajo-core/src/test/resources/queries/TestJoinQuery/testJoinWithOrPredicates.sql
          • tajo-core/src/test/java/org/apache/tajo/engine/query/TestJoinQuery.java
          • tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen2.sql
          • tajo-core/src/test/resources/results/TestJoinQuery/testCrossJoinAndCaseWhen.result
          • CHANGES
          • tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/ProjectionPushDownRule.java
          • tajo-core/src/test/resources/queries/TestJoinQuery/testCrossJoinAndCaseWhen.sql
          Show
          hudson Hudson added a comment - ABORTED: Integrated in Tajo-master-build #633 (See https://builds.apache.org/job/Tajo-master-build/633/ ) TAJO-1350 : Refactor FilterPushDownRule::visitJoin() into well-defined, small methods. (jihoon) (jihoonson: rev b1e174eec4c15142b6fa518b6803579bb0788e8e) tajo-core/src/test/resources/results/TestJoinQuery/testInnerJoinAndCaseWhen.result tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen2.result tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/FilterPushDownRule.java tajo-core/src/test/resources/results/TestJoinQuery/testComplexJoinsWithCaseWhen.result tajo-core/src/test/resources/results/TestJoinQuery/testJoinWithOrPredicates.result tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen.sql tajo-plan/src/main/java/org/apache/tajo/plan/util/PlannerUtil.java tajo-core/src/test/resources/queries/TestJoinQuery/testInnerJoinAndCaseWhen.sql tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java tajo-core/src/test/resources/queries/TestJoinQuery/testJoinWithOrPredicates.sql tajo-core/src/test/java/org/apache/tajo/engine/query/TestJoinQuery.java tajo-core/src/test/resources/queries/TestJoinQuery/testComplexJoinsWithCaseWhen2.sql tajo-core/src/test/resources/results/TestJoinQuery/testCrossJoinAndCaseWhen.result CHANGES tajo-plan/src/main/java/org/apache/tajo/plan/rewrite/rules/ProjectionPushDownRule.java tajo-core/src/test/resources/queries/TestJoinQuery/testCrossJoinAndCaseWhen.sql

            People

            • Assignee:
              jihoonson Jihoon Son
              Reporter:
              hyunsik Hyunsik Choi
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development