You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by sureshsubbiah <gi...@git.apache.org> on 2016/01/29 00:50:16 UTC
[GitHub] incubator-trafodion pull request: [TRAFODION-1791] UPSERT with bar...
GitHub user sureshsubbiah opened a pull request:
https://github.com/apache/incubator-trafodion/pull/291
[TRAFODION-1791] UPSERT with bare one row subquery in VALUES clause fails [TRAFODION-1792] UPSERT with parameters in VALUES clause fails
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sureshsubbiah/incubator-trafodion hive2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/291.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 #291
----
commit c0a2252743958236e060de6ce8287582ea16ab0d
Author: Suresh Subbiah <su...@apache.org>
Date: 2016-01-28T23:43:15Z
[TRAFODION-1791] UPSERT with bare one row subquery in VALUES clause fails
[TRAFODION-1792] UPSERT with parameters in VALUES clause fails
Please see JIRA for explanation of the fix. Fix for 1791 is in NormRelExpr.cpp.
While change in BindRelExpr.cpp is for 1792. Regression tests have been added with a
few different variations.
----
---
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-1791] UPSERT with bar...
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/291#discussion_r51219873
--- Diff: core/sql/optimizer/BindRelExpr.cpp ---
@@ -10272,8 +10273,14 @@ RelExpr* Insert::xformUpsertToMerge(BindWA *bindWA)
((Join*)re)->doNotTransformToTSJ();
((Join*)re)->setTSJForMerge(TRUE);
((Join*)re)->setTSJForMergeWithInsert(TRUE);
+ ((Join*)re)->setTSJForMergeUpsert(TRUE);
((Join*)re)->setTSJForWrite(TRUE);
- if (bindWA->hasDynamicRowsetsInQuery())
+
+ // if Inputs of current insert are empty (i.e. we have no params/rowsets)
+ // then there will be no pull up of inputs during transform and the join will
+ // not see the inputs of the mergeUpdate due to intermediate nodes. So
+ // add inputs directly to join and use elimination to remove extra inputs
+ if (NOT getGroupAttr()->getCharacteristicInputs().isEmpty())
--- End diff --
I'm having trouble understanding this if statement. As far as I can see, _myOuterRefs_ are the source values, coming from the child of the original insert. After the transformation they will come from the left child of the tuple flow, _re_, and be passed to the right child, _mu_. So, I would have added them unconditionally as char. inputs of _mu_. If the insert (_this_) char. inputs are non-empty, then I would have add those to _re_ and maybe even to _mu_.
---
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-1791] UPSERT with bar...
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/291#discussion_r51220665
--- Diff: core/sql/regress/executor/EXPECTED015.SB ---
@@ -1721,4 +1799,70 @@ A B C
--- 1 row(s) selected.
>>
+>>-- guarding against error 7000 and compiler asserts
+>>prepare s1 from UPSERT INTO DEC1
++>(ID, codeValue, title , description, path, synonyms, objectClassCode, propertyCode)
++>VALUES (NVL((select a.id from DEC1 a where a.codeValue = ?a[10]), DEFAULT),
++>?b[10], ?c[10], ?d[10], ?e[10], null, ?f[10], null) ;
+
+--- SQL command prepared.
+>>
+>>prepare s2 from UPSERT INTO DEC1
++>(ID, codeValue, title , description, path, synonyms, objectClassCode, propertyCode)
++>VALUES (?a, ?b, ?c, ?d, ?e, null, ?f, null) ;
+
+--- SQL command prepared.
+>>
+>>prepare s3 from UPSERT INTO DEC1
++>(ID, codeValue, title , description, path, synonyms, objectClassCode, propertyCode)
++>VALUES (NVL((select a.id from DEC1 a where a.codeValue = ?a), DEFAULT),
++>?b, ?c, ?d, ?e, null, ?f, null) ;
+
+--- SQL command prepared.
+>>
+>>prepare s4 from UPSERT INTO DE
++>(ID, dataElementConceptID, valueDomainID) VALUES
++>(NVL((select a.id from DE a where a.dataElementConceptID =
++>(select b.id from DEC1 b where b.codeValue = ?) and a.valueDomainID = ? ), DEFAULT),
++>NVL((select d.id from DEC1 d where d.codeValue = ?), NULL), ? ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s5 from UPSERT INTO ODT
++>(ID, objectTypeID, dataElementID, objectCategoryTypeCode)
++>VALUES (NVL((select a.id from ODT a where a.objectTypeID =
++>(select b.id from OT b where b.objectCode = ? and b.version = ?) and
++>a.objectCategoryTypeCode = ? and a.dataElementID =
++>(select d.id from DE d where d.dataElementConceptID =
++>(select e.id from DEC1 e where e.codeValue = ?) and d.valueDomainID = ? ) ), DEFAULT),
++>NVL((select g.id from OT g where g.objectCode = ? and g.version = ?), NULL),
++>NVL((select h.id from DE h where h.dataElementConceptID =
++>(select i.id from DEC1 i where i.codeValue = ?) and h.valueDomainID = ? ), NULL), ? );
+
+--- SQL command prepared.
+>>
+>>prepare s6 from UPSERT INTO DE
++>(ID, dataElementConceptID, valueDomainID) VALUES
++>(NVL((select a.id from DE a where a.dataElementConceptID =
++>(select b.id from DEC1 b where b.codeValue = ?) and a.valueDomainID = ? ), DEFAULT),
++>(select d.id from DEC1 d where d.codeValue = ?), ? ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s7 from UPSERT INTO DE (ID, dataElementConceptID, valueDomainID)
++>VALUES (1, (select d.id from DEC1 d where d.codeValue = ?), 3 ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s8 from UPSERT INTO DE (ID, dataElementConceptID, valueDomainID)
++>VALUES (?[10], ?[10], ?[10] ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s9 from UPSERT INTO DE (ID, dataElementConceptID, valueDomainID)
++>VALUES (1, (select d.id from DEC1 d where d.codeValue = 'aa'), 3 ) ;
+
--- End diff --
Just surviving the PREPARE of these is good enough for the test? Would it make sense to confirm the transformation with something like this: select count(*) from table(explain(null, 'S9')) where operator like '%MERGE%';
---
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-1791] UPSERT with bar...
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/291#discussion_r51333756
--- Diff: core/sql/optimizer/BindRelExpr.cpp ---
@@ -10272,8 +10273,14 @@ RelExpr* Insert::xformUpsertToMerge(BindWA *bindWA)
((Join*)re)->doNotTransformToTSJ();
((Join*)re)->setTSJForMerge(TRUE);
((Join*)re)->setTSJForMergeWithInsert(TRUE);
+ ((Join*)re)->setTSJForMergeUpsert(TRUE);
((Join*)re)->setTSJForWrite(TRUE);
- if (bindWA->hasDynamicRowsetsInQuery())
+
+ // if Inputs of current insert are empty (i.e. we have no params/rowsets)
+ // then there will be no pull up of inputs during transform and the join will
+ // not see the inputs of the mergeUpdate due to intermediate nodes. So
+ // add inputs directly to join and use elimination to remove extra inputs
+ if (NOT getGroupAttr()->getCharacteristicInputs().isEmpty())
--- End diff --
Thanks for the explanation!
---
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-1791] UPSERT with bar...
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/291#discussion_r51220419
--- Diff: core/sql/optimizer/NormRelExpr.cpp ---
@@ -7050,8 +7077,8 @@ void RelRoot::recomputeOuterReferences()
child(0)->getGroupAttr()->getCharacteristicInputs().
getLeafValuesForCoverTest(leafValues, emptyGA, emptySet);
CMPASSERT((getGroupAttr()->getCharacteristicInputs().contains
- (child(0)->getGroupAttr()->getCharacteristicInputs())) ||
- (getGroupAttr()->getCharacteristicInputs().contains (leafValues)));
+ (child(0)->getGroupAttr()->getCharacteristicInputs())) ||
--- End diff --
Message from the TAB police, if you can, configure your editor to put spaces instead of tabs. The good news is that they are configured as 8 spaces :-)
---
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-1791] UPSERT with bar...
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/291#discussion_r51333778
--- Diff: core/sql/optimizer/NormRelExpr.cpp ---
@@ -7050,8 +7077,8 @@ void RelRoot::recomputeOuterReferences()
child(0)->getGroupAttr()->getCharacteristicInputs().
getLeafValuesForCoverTest(leafValues, emptyGA, emptySet);
CMPASSERT((getGroupAttr()->getCharacteristicInputs().contains
- (child(0)->getGroupAttr()->getCharacteristicInputs())) ||
- (getGroupAttr()->getCharacteristicInputs().contains (leafValues)));
+ (child(0)->getGroupAttr()->getCharacteristicInputs())) ||
--- End diff --
You can add this to your ~/.emacs file:
(setq-default indent-tabs-mode nil)
---
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-1791] UPSERT with bar...
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/291#discussion_r51330150
--- Diff: core/sql/regress/executor/EXPECTED015.SB ---
@@ -1721,4 +1799,70 @@ A B C
--- 1 row(s) selected.
>>
+>>-- guarding against error 7000 and compiler asserts
+>>prepare s1 from UPSERT INTO DEC1
++>(ID, codeValue, title , description, path, synonyms, objectClassCode, propertyCode)
++>VALUES (NVL((select a.id from DEC1 a where a.codeValue = ?a[10]), DEFAULT),
++>?b[10], ?c[10], ?d[10], ?e[10], null, ?f[10], null) ;
+
+--- SQL command prepared.
+>>
+>>prepare s2 from UPSERT INTO DEC1
++>(ID, codeValue, title , description, path, synonyms, objectClassCode, propertyCode)
++>VALUES (?a, ?b, ?c, ?d, ?e, null, ?f, null) ;
+
+--- SQL command prepared.
+>>
+>>prepare s3 from UPSERT INTO DEC1
++>(ID, codeValue, title , description, path, synonyms, objectClassCode, propertyCode)
++>VALUES (NVL((select a.id from DEC1 a where a.codeValue = ?a), DEFAULT),
++>?b, ?c, ?d, ?e, null, ?f, null) ;
+
+--- SQL command prepared.
+>>
+>>prepare s4 from UPSERT INTO DE
++>(ID, dataElementConceptID, valueDomainID) VALUES
++>(NVL((select a.id from DE a where a.dataElementConceptID =
++>(select b.id from DEC1 b where b.codeValue = ?) and a.valueDomainID = ? ), DEFAULT),
++>NVL((select d.id from DEC1 d where d.codeValue = ?), NULL), ? ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s5 from UPSERT INTO ODT
++>(ID, objectTypeID, dataElementID, objectCategoryTypeCode)
++>VALUES (NVL((select a.id from ODT a where a.objectTypeID =
++>(select b.id from OT b where b.objectCode = ? and b.version = ?) and
++>a.objectCategoryTypeCode = ? and a.dataElementID =
++>(select d.id from DE d where d.dataElementConceptID =
++>(select e.id from DEC1 e where e.codeValue = ?) and d.valueDomainID = ? ) ), DEFAULT),
++>NVL((select g.id from OT g where g.objectCode = ? and g.version = ?), NULL),
++>NVL((select h.id from DE h where h.dataElementConceptID =
++>(select i.id from DEC1 i where i.codeValue = ?) and h.valueDomainID = ? ), NULL), ? );
+
+--- SQL command prepared.
+>>
+>>prepare s6 from UPSERT INTO DE
++>(ID, dataElementConceptID, valueDomainID) VALUES
++>(NVL((select a.id from DE a where a.dataElementConceptID =
++>(select b.id from DEC1 b where b.codeValue = ?) and a.valueDomainID = ? ), DEFAULT),
++>(select d.id from DEC1 d where d.codeValue = ?), ? ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s7 from UPSERT INTO DE (ID, dataElementConceptID, valueDomainID)
++>VALUES (1, (select d.id from DEC1 d where d.codeValue = ?), 3 ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s8 from UPSERT INTO DE (ID, dataElementConceptID, valueDomainID)
++>VALUES (?[10], ?[10], ?[10] ) ;
+
+--- SQL command prepared.
+>>
+>>prepare s9 from UPSERT INTO DE (ID, dataElementConceptID, valueDomainID)
++>VALUES (1, (select d.id from DEC1 d where d.codeValue = 'aa'), 3 ) ;
+
--- End diff --
Will make this change in my next delivery.
---
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-1791] UPSERT with bar...
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/291#discussion_r51220131
--- Diff: core/sql/optimizer/NormRelExpr.cpp ---
@@ -1697,11 +1709,26 @@ void Join::recomputeOuterReferences()
// If it is a TSJ don't add the outputs of the left child to
--- End diff --
Could also fix the comment.
---
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-1791] UPSERT with bar...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/291
---
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-1791] UPSERT with bar...
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/291#discussion_r51330128
--- Diff: core/sql/optimizer/NormRelExpr.cpp ---
@@ -7050,8 +7077,8 @@ void RelRoot::recomputeOuterReferences()
child(0)->getGroupAttr()->getCharacteristicInputs().
getLeafValuesForCoverTest(leafValues, emptyGA, emptySet);
CMPASSERT((getGroupAttr()->getCharacteristicInputs().contains
- (child(0)->getGroupAttr()->getCharacteristicInputs())) ||
- (getGroupAttr()->getCharacteristicInputs().contains (leafValues)));
+ (child(0)->getGroupAttr()->getCharacteristicInputs())) ||
--- End diff --
Will fix my TABs, likely after calling you. I use emacs and am not sure how to set this correctly.
---
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-1791] UPSERT with bar...
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/291#discussion_r51330090
--- Diff: core/sql/optimizer/BindRelExpr.cpp ---
@@ -10272,8 +10273,14 @@ RelExpr* Insert::xformUpsertToMerge(BindWA *bindWA)
((Join*)re)->doNotTransformToTSJ();
((Join*)re)->setTSJForMerge(TRUE);
((Join*)re)->setTSJForMergeWithInsert(TRUE);
+ ((Join*)re)->setTSJForMergeUpsert(TRUE);
((Join*)re)->setTSJForWrite(TRUE);
- if (bindWA->hasDynamicRowsetsInQuery())
+
+ // if Inputs of current insert are empty (i.e. we have no params/rowsets)
+ // then there will be no pull up of inputs during transform and the join will
+ // not see the inputs of the mergeUpdate due to intermediate nodes. So
+ // add inputs directly to join and use elimination to remove extra inputs
+ if (NOT getGroupAttr()->getCharacteristicInputs().isEmpty())
--- End diff --
In RelExpr::recomputeOuterReferences and most other implementations of recomputeOuterReferences char inputs are pulled up from the child to the parent, but this pullup code is executed only if the parent has inputs to start with. If parent has no inputs to start with, then most recomputeOuterRefences implementations do little. I was seeing that if inputs were added to "mu" and the UPSERT had no params (i.e. when all inputs mentioned in this method are internal ones, those that are required because the "mu" is now on the right side of a tuple_flow, not those that will come in from the cli through the input_desc) they were not getting pulled up, and inlining nodes were not getting the values they needed.
I tried for a long to time to avoid adding any inputs explicitly to both "mu" and "re" and letting inputs be managed by existing code in Transform/Normalize. I could not get that to work as they started failing an assert in optimizer. Now I have adopted the idea of doing the minimum necessary. Add only to "mu" as far as possible, however in the no input_desc char. inputs I did have to add it to "re".
---
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.
---