You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by DaveBirdsall <gi...@git.apache.org> on 2015/11/14 00:18:34 UTC
[GitHub] incubator-trafodion pull request: [TRAFODION-1585] Allow MDAM on i...
GitHub user DaveBirdsall opened a pull request:
https://github.com/apache/incubator-trafodion/pull/174
[TRAFODION-1585] Allow MDAM on inner child of NJ in certain circumsta…
…nces. Details are given in the JIRA.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaveBirdsall/incubator-trafodion Trafodion1585Fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/174.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 #174
----
commit a9dea6bf427d91a0e1a69c4d89e473a4c3658dc1
Author: Dave Birdsall <db...@apache.org>
Date: 2015-11-13T23:15:03Z
[TRAFODION-1585] Allow MDAM on inner child of NJ in certain circumstances
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1585] Allow MDAM on i...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/174#discussion_r45015547
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -3910,6 +3910,20 @@ InputPhysicalProperty* NestedJoin::generateIpp(
InputPhysicalProperty* ipp = NULL;
// ----------------------------------------------------------------
+ // If there is a cardinality constraint on the outer child, we'll
+ // pass that information along to the inner child.
+ // ----------------------------------------------------------------
+ Cardinality minRows;
+ Cardinality maxRows;
+ CardConstraint * outerCardinalityConstraint = NULL;
+
+ if (getGroupAttr()->hasCardConstraint(minRows, maxRows))
--- End diff --
I would have expected that this would examine the join's left child's group attributes, not those of the join itself?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1585] Allow MDAM on i...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/174#discussion_r45080794
--- Diff: core/sql/optimizer/ScanOptimizer.cpp ---
@@ -3364,7 +3364,25 @@ ScanOptimizer::isMdamEnabled() const
// If # of probes for NJ is <= cqd(MDAM_UNDER_NJ_PROBES_THRESHOLD),
// we will use MDAM regardless of the setting of CQD MDAM_SCAN_METHOD.
- // When the CQD is 0, MDAM under NJ is not considered.
+ // When the CQD is 0, MDAM under NJ is not considered unless we
+ // have a cardinality constraint on the number of probes that
+ // guarantees at most one probe.
+
+ const InputPhysicalProperty* ippForMe =
+ getContext().getInputPhysicalProperty();
+ if (ippForMe)
+ {
+ const CardConstraint * outerCardConstraint =
+ ippForMe->getOuterCardConstraint();
+ if (outerCardConstraint)
+ {
+ Cardinality upperBound = outerCardConstraint->getUpperBound();
+ if (upperBound <= (Cardinality)1)
--- End diff --
There is already a CQD just a few lines down in the code that will allow this for estimates. I wanted to avoid introducing yet another CQD. That was the point of the code a few lines down that used the cardinality constraint to sharpen the row estimate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1585] Allow MDAM on i...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/174#discussion_r45124335
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -3910,6 +3910,20 @@ InputPhysicalProperty* NestedJoin::generateIpp(
InputPhysicalProperty* ipp = NULL;
// ----------------------------------------------------------------
+ // If there is a cardinality constraint on the outer child, we'll
+ // pass that information along to the inner child.
+ // ----------------------------------------------------------------
+ Cardinality minRows;
+ Cardinality maxRows;
+ CardConstraint * outerCardinalityConstraint = NULL;
+
+ if (getGroupAttr()->hasCardConstraint(minRows, maxRows))
--- End diff --
Yes, the intent of the change is to send any cardinality constraint on the result of the left child down to the right child scan as a constraint on the number of probes. Before this change, no such constraint was passed down; we relied on estimates only. The difference is a matter of certainty. MDAM was considered dangerous to do on the inner child of a nested join due to unreliability of estimates in some plans.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1585] Allow MDAM on i...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/174#discussion_r45015567
--- Diff: core/sql/optimizer/ScanOptimizer.cpp ---
@@ -3364,7 +3364,25 @@ ScanOptimizer::isMdamEnabled() const
// If # of probes for NJ is <= cqd(MDAM_UNDER_NJ_PROBES_THRESHOLD),
// we will use MDAM regardless of the setting of CQD MDAM_SCAN_METHOD.
- // When the CQD is 0, MDAM under NJ is not considered.
+ // When the CQD is 0, MDAM under NJ is not considered unless we
+ // have a cardinality constraint on the number of probes that
+ // guarantees at most one probe.
+
+ const InputPhysicalProperty* ippForMe =
+ getContext().getInputPhysicalProperty();
+ if (ippForMe)
+ {
+ const CardConstraint * outerCardConstraint =
+ ippForMe->getOuterCardConstraint();
+ if (outerCardConstraint)
+ {
+ Cardinality upperBound = outerCardConstraint->getUpperBound();
+ if (upperBound <= (Cardinality)1)
--- End diff --
Sounds good for now, maybe we can extend this to small cardinalities > 1 in the future, e.g. by using an integer-valued CQD here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1585] Allow MDAM on i...
Posted by sureshsubbiah <gi...@git.apache.org>.
Github user sureshsubbiah commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/174#discussion_r45081166
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -3910,6 +3910,20 @@ InputPhysicalProperty* NestedJoin::generateIpp(
InputPhysicalProperty* ipp = NULL;
// ----------------------------------------------------------------
+ // If there is a cardinality constraint on the outer child, we'll
+ // pass that information along to the inner child.
+ // ----------------------------------------------------------------
+ Cardinality minRows;
+ Cardinality maxRows;
+ CardConstraint * outerCardinalityConstraint = NULL;
+
+ if (getGroupAttr()->hasCardConstraint(minRows, maxRows))
--- End diff --
Maybe in the case described in TRAFODION-1585 the CardConstraint on the outer child, resulted in a CardConstraint being synthesized for the Join too? (since the Join is on key columns?), hence this change produced the desired plan.
That raises the question on whether the CardConstraint that is sent down to the inner child states that
a) The number of probes or unique values for input from outer is limited by the CradConstraint
or b) whether the output of the inner child satisfies the CardConstraint. This is a basic question, but my knowledge of how Ipp works is limited.
Also do we need to consider inputs or probes that could come from the top. I am thinking that the right child of an NJ can see several requests, even though the left child send only one probe for each request that it receives.
All the questions I have are minor and for my personal knowledge, I think the change is good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1585] Allow MDAM on i...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/174#discussion_r45080536
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -3910,6 +3910,20 @@ InputPhysicalProperty* NestedJoin::generateIpp(
InputPhysicalProperty* ipp = NULL;
// ----------------------------------------------------------------
+ // If there is a cardinality constraint on the outer child, we'll
+ // pass that information along to the inner child.
+ // ----------------------------------------------------------------
+ Cardinality minRows;
+ Cardinality maxRows;
+ CardConstraint * outerCardinalityConstraint = NULL;
+
+ if (getGroupAttr()->hasCardConstraint(minRows, maxRows))
--- End diff --
Good catch... I think you are right. I will fix this. I wondered why my unit test succeeded with this mistake. It succeeded because both legs of the join had cardinality constraint of one so the join itself had this constraint also.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---