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 2018/01/25 00:27:33 UTC
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
GitHub user DaveBirdsall opened a pull request:
https://github.com/apache/trafodion/pull/1414
[TRAFODION-2840] Make [first n] with ORDER BY views non-updatable
JIRA TRAFODION-2822 attempted to make views containing [first n] in the select list non-updatable. It missed one case, the case where both [first n] and ORDER BY are present in the view definition.
The reason it missed that case is because of a design oddity in how [first n] is implemented. When there is no ORDER BY clause, the Binder pass introduces a FirstN node into the query tree. But if an ORDER BY is present, it defers doing this to the Generator. That means any FirstN-related semantic checks in earlier passes will not be applied when ORDER BY is present.
This set of changes refactors the FirstN implementation a little bit so that the FirstN can be introduced in the Binder when ORDER BY is present. Then the check added in JIRA TRAFODION-2822 will apply, and such views will be marked non-updatable as desired.
The refactored design copies a bound and transformed version of the ORDER BY clause into the FirstN node at Normalize time. (Aside: I tried copying the unbound ORDER BY tree into the FirstN node at Bind time initially, but this fails if there are expressions in the select list referenced by the ORDER BY; different ValueIds get assigned to parts of the expressions than in the original resulting in expression coverage check failures at Optimize time.)
The refactored design has been implemented for all code paths that are involved in view definition, which is all that is required to satisfy this JIRA. However, there is another code path that hasn't been addressed: If a query uses output rowsets, has [first n] + ORDER BY, then the FirstN is still added in the Generator. I took a look at fixing this one too, but it proved more tedious than I wanted to tackle just now. A follow-on JIRA, TRAFODION-2924, has been written to track that case.
In addition, I noticed some code paths that use [first n] today that do not work correctly. The parser supports [first n] syntax on UPDATE and DELETE statements. But I discovered (even before my changes) that UPDATE ignores its [first n] specification. And if [first n] is specified for DELETE, the query fails with a compiler error 2235 (cannot produce plan). So those evidently are incomplete features, or features that broke sometime in the past and no-one noticed until now.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaveBirdsall/trafodion Trafodion2840
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1414.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 #1414
----
commit ebf7283de994729f898fa5a9f5e476fa03b40a4f
Author: Dave Birdsall <db...@...>
Date: 2018-01-25T00:16:08Z
[TRAFODION-2840] Make [first n] with ORDER BY views non-updatable
----
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163930312
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
--- End diff --
Hmmmm... Suppose the parent is different at normalize time than it is here. (I don't think that ever happens but let's suppose in the future some Optimizer rule might make that happen.) Suppose further that the order requirement for the parent is different. We can generate a valid plan by doing a sort before doing first n (satisfying the first n + ORDER BY semantic) and then doing another sort with different criteria afterward (satisfying the different sort order of the parent). I think that's what this code achieves. If I remove this line, then we won't generate a plan (which might be OK because there might be other plan alternatives, but it might not in which case we'll get an error 2235). So my take is that leaving this line here is slightly more robust. What do you think?
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163894233
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
--- End diff --
I don't think we should remove the sort requirement from the parent here, since the FirstN does not itself do any sorting, so whatever the parent needs will have to be provided by the child. Therefore we need to pass the parent's requirement on to the child. If what the parent wants is incompatible with what the FirstN needs (probably not possible in the current design), we'll return NULL on line 15562, which is what we want.
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163941065
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
--- End diff --
If the parent really produces a different order requirement, I agree that the solution is two sorts. Line 15532 does not achieve that, however. To get two sorts, we need to wait until we have a sort as a parent, and that sort will not require a sort order. Leaving the line in may result in an inconsistent plan that will be ignored by the Cascades engine.
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163930378
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
+
+ ValueIdList sortKey;
+ sortKey.insert(reqdOrder());
--- End diff --
Will fix. Thanks. I think I copied this code from another method.
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163895532
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
+
+ ValueIdList sortKey;
+ sortKey.insert(reqdOrder());
+
+ // Shouldn't/Can't add a sort order type requirement
+ // if we are in DP2
+ if (rppForMe->executeInDP2())
+ rg.addSortKey(sortKey,NO_SOT);
+ else
+ rg.addSortKey(sortKey,ESP_SOT);
+ }
+
+ if (NOT pws->isEmpty())
--- End diff --
I don't think we need this code (line 15545 - 15559). We already returned from this method if pws contains previous contexts (line 15524), so this if condition should never be true.
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1414
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163930862
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
+
+ ValueIdList sortKey;
+ sortKey.insert(reqdOrder());
+
+ // Shouldn't/Can't add a sort order type requirement
+ // if we are in DP2
+ if (rppForMe->executeInDP2())
+ rg.addSortKey(sortKey,NO_SOT);
+ else
+ rg.addSortKey(sortKey,ESP_SOT);
+ }
+
+ if (NOT pws->isEmpty())
--- End diff --
I think you're right. Perhaps this is one of those code blocks I copied from the default implementation that gets executed only when arity > 1? I'll look into it more carefully and remove this if it is dead code.
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163898763
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
+
+ ValueIdList sortKey;
+ sortKey.insert(reqdOrder());
--- End diff --
Just a nit: It isn't really necessary to make a copy of the required order here, reqdOrder() could be passed directly to the rg.addSortKey method below?
---
[GitHub] trafodion pull request #1414: [TRAFODION-2840] Make [first n] with ORDER BY ...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1414#discussion_r163943604
--- Diff: core/sql/optimizer/OptPhysRelExpr.cpp ---
@@ -15499,6 +15499,95 @@ GenericUtilExpr::synthPhysicalProperty(const Context* myContext,
return sppForMe;
} // GenericUtilExpr::synthPhysicalProperty()
+// -----------------------------------------------------------------------
+// FirstN::createContextForAChild()
+// The FirstN node may have an order by requirement that it needs to
+// pass to its child context. Other than that, this method is quite
+// similar to the default implementation, RelExpr::createContextForAChild.
+// The arity of FirstN is always 1, so some logic from the default
+// implementation that deals with childIndex > 0 is unnecessary and has
+// been removed.
+// -----------------------------------------------------------------------
+Context * FirstN::createContextForAChild(Context* myContext,
+ PlanWorkSpace* pws,
+ Lng32& childIndex)
+{
+ const ReqdPhysicalProperty* rppForMe =
+ myContext->getReqdPhysicalProperty();
+
+ CMPASSERT(getArity() == 1);
+
+ childIndex = getArity() - pws->getCountOfChildContexts() - 1;
+
+ // return if we are done
+ if (childIndex < 0)
+ return NULL;
+
+ RequirementGenerator rg(child(childIndex), rppForMe);
+
+ if (reqdOrder().entries() > 0)
+ {
+ // replace our sort requirement with that implied by our ORDER BY clause
+
+ rg.removeSortKey();
--- End diff --
Gotcha. Will remove the line.
---