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 2017/10/03 16:16:32 UTC
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
GitHub user DaveBirdsall opened a pull request:
https://github.com/apache/incubator-trafodion/pull/1253
[TRAFODION-1610][TRAFODION-2630] Generate constr exprs on update operators
This set of changes plugs a hole in our implementation of update/upsert/merge operators. It completes work started by JIRA TRAFODION-1575.
This set of changes adds logic to the generator to generate update constraint check and insert constraint check expressions for update operators. (The update constraint check handles the run-time update path; the insert constraint check handles the run-time insert path for upsert and merge; it is absent for vanilla update and for merges that lack a "when not matched" clause.)
This set of changes also removes temporary code added long ago that transformed an UPDATE to an INSERT + a DELETE when constraint expressions were present. (The INSERT operator had a correct implementation of constraint checking.) This should make many non-key-column UPDATEs faster, and will also eliminate the possibility of HBase timestamp collisions because the INSERT happens within a millisecond of the DELETE.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaveBirdsall/incubator-trafodion Trafodion1610
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/1253.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 #1253
----
commit c22b6d10fdd3065a948163d64a20c3039128474c
Author: Dave Birdsall <db...@apache.org>
Date: 2017-10-03T16:09:29Z
[TRAFODION-1610][TRAFODION-2630] Generate constr exprs on update operators
----
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142454172
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NAColumn *targetCol = NULL;
--- End diff --
Just noticed something that could be improved. It would be better to set targetCol to NULL just inside the "for" loop, so that the "if" at line 329 would not be taken. It's not an observable bug, though, because even if on the second time around, targetCol were pointing to an NAColumn from the first time around, the other comparisons at line 330 onward would come out false. Nevertheless, making the change would make it cleaner.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142806660
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -1718,19 +1806,56 @@ short HbaseUpdate::codeGen(Generator * generator)
}
else if (getIndexDesc()->isClusteringIndex() && getCheckConstraints().entries())
{
- GenAssert(FALSE, "Should not reach here. This update should have been transformed to delete/insert");
- // To be uncommented when TRAFODION-1610 is implemented
- // Need to generate insConstraintExpr also
-/*
+ // Generate the update and insert constraint check expressions
+
+ // The attributes for the columns referenced in the constraint expressions
+ // refer to the source values of the columns. We want to evaluate the
+ // constraints aganst the target values, though. So, there is some
+ // Attributes gymnastics that has to happen to generate them.
+
+ // Obtain the ValueIds of base table columns referenced in the
+ // constraints
+
+ ValueId constraintId;
+ ValueIdSet constraintColumns;
+ for (CollIndex ci = 0; ci < getCheckConstraints().entries(); ci++)
+ {
+ constraintId = getCheckConstraints()[ci];
+ constraintId.getItemExpr()->findAll(ITM_INDEXCOLUMN,
+ constraintColumns, // out, has append semantics
+ TRUE, // visitVEGmembers
+ FALSE); // don't visit index descriptors
+ }
+
+ // Prepare the constraint tree for generation
+
ItemExpr *constrTree =
getCheckConstraints().rebuildExprTree(ITM_AND, TRUE, TRUE);
if (getTableDesc()->getNATable()->hasSerializedEncodedColumn())
- constrTree = generator->addCompDecodeForDerialization(constrTree);
+ constrTree = generator->addCompDecodeForDerialization(constrTree, isAlignedFormat);
+
+ // Generate the update constraint expression, substituting Attributes for
--- End diff --
Should have edited this comment too
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142546324
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
--- End diff --
Yes, sorry to comment on surround code. If both source and target share the same NATable, then wouldn't the following be sufficient:
```c++
if ((targetCol) && courceCol == targetCol)
...
```
Another option would be to compare their column number:
```c++
CMPASSERT(sourceCol->getNATable() == targetCol->getNATable());
if (targetCol && sourceCol->getPosition() == targetCol->getPosition())
...
```
One thing might be a problem: I wonder whether in some cases we might suppress caching of the NATable on the insert side and allow it on the select side or vice versa.
I wonder whether @sureshsubbiah has any additional comments on this topic.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142539746
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
+ savedValAttr->copyLocationAttrs(sourceValAttr);
+ savedSourceAttrsList.insert(savedValAttr);
+ savedSourceVIDlist.insert(sourceValId);
+
+ sourceValAttr->copyLocationAttrs(targetValAttr);
+ }
+
+ }
+
+ // Now that we have remapped the Attributes for the columns to their values
+ // in the new record, we can generate the update constraint expression.
+
+ expGen->generateExpr(constrTree->getValueId(), ex_expr::exp_SCAN_PRED,
--- End diff --
Good question. I will investigate.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142806590
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -1718,19 +1806,56 @@ short HbaseUpdate::codeGen(Generator * generator)
}
else if (getIndexDesc()->isClusteringIndex() && getCheckConstraints().entries())
{
- GenAssert(FALSE, "Should not reach here. This update should have been transformed to delete/insert");
- // To be uncommented when TRAFODION-1610 is implemented
- // Need to generate insConstraintExpr also
-/*
+ // Generate the update and insert constraint check expressions
+
+ // The attributes for the columns referenced in the constraint expressions
+ // refer to the source values of the columns. We want to evaluate the
+ // constraints aganst the target values, though. So, there is some
+ // Attributes gymnastics that has to happen to generate them.
--- End diff --
Or at least edited them
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142539579
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
--- End diff --
Happy to consider. Can you point me to an instructive example?
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142806685
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -1718,19 +1806,56 @@ short HbaseUpdate::codeGen(Generator * generator)
}
else if (getIndexDesc()->isClusteringIndex() && getCheckConstraints().entries())
{
- GenAssert(FALSE, "Should not reach here. This update should have been transformed to delete/insert");
- // To be uncommented when TRAFODION-1610 is implemented
- // Need to generate insConstraintExpr also
-/*
+ // Generate the update and insert constraint check expressions
+
+ // The attributes for the columns referenced in the constraint expressions
+ // refer to the source values of the columns. We want to evaluate the
+ // constraints aganst the target values, though. So, there is some
+ // Attributes gymnastics that has to happen to generate them.
+
+ // Obtain the ValueIds of base table columns referenced in the
+ // constraints
+
+ ValueId constraintId;
+ ValueIdSet constraintColumns;
+ for (CollIndex ci = 0; ci < getCheckConstraints().entries(); ci++)
+ {
+ constraintId = getCheckConstraints()[ci];
+ constraintId.getItemExpr()->findAll(ITM_INDEXCOLUMN,
+ constraintColumns, // out, has append semantics
+ TRUE, // visitVEGmembers
+ FALSE); // don't visit index descriptors
+ }
+
+ // Prepare the constraint tree for generation
+
ItemExpr *constrTree =
getCheckConstraints().rebuildExprTree(ITM_AND, TRUE, TRUE);
if (getTableDesc()->getNATable()->hasSerializedEncodedColumn())
- constrTree = generator->addCompDecodeForDerialization(constrTree);
+ constrTree = generator->addCompDecodeForDerialization(constrTree, isAlignedFormat);
+
+ // Generate the update constraint expression, substituting Attributes for
+ // the new update record
+
+ genUpdConstraintExpr(generator,
+ constrTree,
+ constraintColumns,
+ newRecExprArray(),
+ &updConstraintExpr /* out */);
+
+ if ((isMerge()) && (mergeInsertRecExprArray().entries() > 0))
+ {
+ // Generate the insert constraint expression, substituting Attributes for
--- End diff --
And this one
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142513113
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
--- End diff --
An assert that the ItemExpr here is really an index column might be good.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/1253
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142540125
--- Diff: core/sql/optimizer/BindRelExpr.cpp ---
@@ -11552,35 +11552,7 @@ RelExpr *Update::bindNode(BindWA *bindWA)
NABoolean transformUpdateKey = updatesClusteringKeyOrUniqueIndexKey(bindWA);
if (bindWA->errStatus()) // error occurred in updatesCKOrUniqueIndexKey()
return this;
- // To be removed when TRAFODION-1610 is implemented.
- NABoolean xnsfrmHbaseUpdate = FALSE;
- if ((hbaseOper()) && (NOT isMerge()))
- {
- if (CmpCommon::getDefault(HBASE_TRANSFORM_UPDATE_TO_DELETE_INSERT) == DF_ON)
--- End diff --
You are correct! Good catch. I will remove it.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142527381
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
--- End diff --
This code to find the corresponding column seems risky to me. Isn't there a way to find the column by position? If the NATables for source and target are the same, that should be possible. If they are not, then matching by name is probably not the right thing anyway.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142539058
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
--- End diff --
Thanks. Will add.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142530211
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
+ savedValAttr->copyLocationAttrs(sourceValAttr);
+ savedSourceAttrsList.insert(savedValAttr);
+ savedSourceVIDlist.insert(sourceValId);
+
+ sourceValAttr->copyLocationAttrs(targetValAttr);
+ }
+
+ }
+
+ // Now that we have remapped the Attributes for the columns to their values
+ // in the new record, we can generate the update constraint expression.
+
+ expGen->generateExpr(constrTree->getValueId(), ex_expr::exp_SCAN_PRED,
--- End diff --
I don't quite understand what happens when the constraint tree refers to columns that are not in the assign list, or is the assign list targetRecExpr always complete? If it's always complete, there would be an optimization to skip constraints that don't refer to any updated columns.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142805915
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -1718,19 +1806,56 @@ short HbaseUpdate::codeGen(Generator * generator)
}
else if (getIndexDesc()->isClusteringIndex() && getCheckConstraints().entries())
{
- GenAssert(FALSE, "Should not reach here. This update should have been transformed to delete/insert");
- // To be uncommented when TRAFODION-1610 is implemented
- // Need to generate insConstraintExpr also
-/*
+ // Generate the update and insert constraint check expressions
+
+ // The attributes for the columns referenced in the constraint expressions
+ // refer to the source values of the columns. We want to evaluate the
+ // constraints aganst the target values, though. So, there is some
+ // Attributes gymnastics that has to happen to generate them.
--- End diff --
Are the Attributes gymnastics gone with the new code? Is it now all couch potatoes?
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142529491
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
--- End diff --
This is probably existing code that's modified, but I still have a comment: Instead of altering the map tables, it might be better to make a ValueIdMap m for the old and new values and calling m.rewriteValueIdUp(newValId, constrTree->getValueId()).
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142531876
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
+ savedValAttr->copyLocationAttrs(sourceValAttr);
+ savedSourceAttrsList.insert(savedValAttr);
+ savedSourceVIDlist.insert(sourceValId);
+
+ sourceValAttr->copyLocationAttrs(targetValAttr);
+ }
+
+ }
+
+ // Now that we have remapped the Attributes for the columns to their values
+ // in the new record, we can generate the update constraint expression.
+
+ expGen->generateExpr(constrTree->getValueId(), ex_expr::exp_SCAN_PRED,
+ targetExpr);
+
+ // Now put the Attributes back the way they were.
+
+ for (Lng32 i = 0; i < savedSourceVIDlist.entries(); i++)
+ {
+ ValueId sourceValId = savedSourceVIDlist[i];
+ Attributes * sourceValAttr = (generator->getMapInfo(sourceValId, 0))->getAttr();
+ sourceValAttr->copyLocationAttrs(savedSourceAttrsList[i]);
--- End diff --
If we created a new map table entry on line 343, this code won't delete that entry. As far as I can tell, it would put back the temporary location that was assigned for a new entry? See comment above, I would recommend not modifying map tables. If you want to stay with the MapTable method, though, it might be better to drop those entries that were created on line 343 here.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142539459
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
--- End diff --
I used the same technique that the existing code used. I'm curious how source and target tables could differ... check constraints are always local to a table, correct? That said, I'm happy to consider alternatives. Can you point me to an example?
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142544778
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
--- End diff --
Thanks. Here is one example, but there are many others: FileScan::preCodeGen() in file GenPreCode.cpp:
```C++
ValueIdMap divExprMap;
ValueId computedBeg;
// If hashJoinBeg is :sysHV1 and the computed column
// expression is A/100, then the begin value for
// the computed column is :sysHV1/100. Do this
// rewrite by using a ValueIdMap
divExprMap.addMapEntry(underlyingCol, hashJoinBeg);
divExprMap.rewriteValueIdDown(computedColExpr->getValueId(),
computedBeg);
newBeg = computedBeg.getItemExpr();
```
There are also examples in file OptPhysRelExpr where we rewrite expressions on the select side of an insert/select into expressions on the insert side, using the ValueIdMap that can be accessed with Join::updateToSelectMap().
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142711212
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
+ savedValAttr->copyLocationAttrs(sourceValAttr);
+ savedSourceAttrsList.insert(savedValAttr);
+ savedSourceVIDlist.insert(sourceValId);
+
+ sourceValAttr->copyLocationAttrs(targetValAttr);
+ }
+
+ }
+
+ // Now that we have remapped the Attributes for the columns to their values
+ // in the new record, we can generate the update constraint expression.
+
+ expGen->generateExpr(constrTree->getValueId(), ex_expr::exp_SCAN_PRED,
--- End diff --
I stepped through the code for an example of this. In the example I looked at, it appears the assign list is complete. My example was an unsalted table with three columns, and I did a MERGE that updated one column, with a set clause of C = 'GLAD'. I found that the assign list for the update record had all three columns in it:
(gdb) p targetRecExprArray.entries()
$2 = 3
(gdb) p targetRecExprArray[0].getItemExpr()->display()
(TRAFODION.SCH.T2.A ASSIGN TRAFODION.SCH.T2.A)
$3 = void
(gdb) p targetRecExprArray[1].getItemExpr()->display()
(TRAFODION.SCH.T2.B ASSIGN TRAFODION.SCH.T2.B)
$4 = void
(gdb) p targetRecExprArray[2].getItemExpr()->display()
(TRAFODION.SCH.T2.C ASSIGN %('GLAD'))
$5 = void
(gdb)
In a nutshell, then, the constraint expression is generated whole, and the columns that aren't affected by the update simply refer to their old values.
It does seem like there is an optimization opportunity here: If the constraint expression doesn't refer to a changed column, we could omit it. If it is not too much trouble, I will do this.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142722721
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -292,8 +292,85 @@ static short genUpdExpr(
return 0;
}
-static short genUpdConstraintExpr(Generator *generator)
+// Used to generate update or insert constraint expressions for update operators
+static short genUpdConstraintExpr(Generator * generator,
+ ItemExpr * constrTree,
+ const ValueIdSet & constraintColumns,
+ ValueIdArray & targetRecExprArray,
+ ex_expr ** targetExpr /* out */)
{
+ ExpGenerator * expGen = generator->getExpGenerator();
+
+ // The Attributes for the table columns refer to the old values of the column.
+ // The constraints must operate on the new values, though. So we must do a
+ // switcheroo on the Attributes for the update expression. The target value IDs
+ // come from targetRecExprArray.
+
+ ValueIdList savedSourceVIDlist;
+ NAList<Attributes*> savedSourceAttrsList(generator->wHeap());
+
+ for (ValueId sourceValId = constraintColumns.init();
+ constraintColumns.next(sourceValId);
+ constraintColumns.advance(sourceValId))
+ {
+ NAColumn * sourceCol = ((IndexColumn*)sourceValId.getItemExpr())->getNAColumn();
+ ValueId targetValId;
+ NABoolean found = FALSE;
+ for (CollIndex ni = 0; (!found) && (ni < targetRecExprArray.entries()); ni++)
+ {
+ const ItemExpr *assignExpr = targetRecExprArray[ni].getItemExpr();
+ targetValId = assignExpr->child(0)->castToItemExpr()->getValueId();
+ NAColumn *targetCol = NULL;
+ if (targetValId.getItemExpr()->getOperatorType() == ITM_BASECOLUMN)
+ targetCol = ((BaseColumn*)targetValId.getItemExpr())->getNAColumn();
+ else if (targetValId.getItemExpr()->getOperatorType() == ITM_INDEXCOLUMN)
+ targetCol = ((IndexColumn*)targetValId.getItemExpr())->getNAColumn();
+
+ if ((targetCol) &&
+ (targetCol->getColName() == sourceCol->getColName()) &&
+ (targetCol->getHbaseColFam() == sourceCol->getHbaseColFam()) &&
+ (targetCol->getHbaseColQual() == sourceCol->getHbaseColQual()) &&
+ (targetCol->getNATable()->getTableName().getQualifiedNameAsAnsiString() ==
+ sourceCol->getNATable()->getTableName().getQualifiedNameAsAnsiString()))
+ {
+ found = TRUE;
+ break;
+ }
+ }
+
+ if (found)
+ {
+ Attributes * sourceValAttr = (generator->addMapInfo(sourceValId, 0))->getAttr();
+ Attributes * targetValAttr = (generator->getMapInfo(targetValId, 0))->getAttr();
+
+ // Save original location attributes so we can change them back after
+ // generating the update constraint expression
+
+ Attributes * savedValAttr = new(generator->wHeap()) Attributes();
+ savedValAttr->copyLocationAttrs(sourceValAttr);
+ savedSourceAttrsList.insert(savedValAttr);
+ savedSourceVIDlist.insert(sourceValId);
+
+ sourceValAttr->copyLocationAttrs(targetValAttr);
+ }
+
+ }
+
+ // Now that we have remapped the Attributes for the columns to their values
+ // in the new record, we can generate the update constraint expression.
+
+ expGen->generateExpr(constrTree->getValueId(), ex_expr::exp_SCAN_PRED,
--- End diff --
Thanks for validating this.
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142534021
--- Diff: core/sql/optimizer/BindRelExpr.cpp ---
@@ -11552,35 +11552,7 @@ RelExpr *Update::bindNode(BindWA *bindWA)
NABoolean transformUpdateKey = updatesClusteringKeyOrUniqueIndexKey(bindWA);
if (bindWA->errStatus()) // error occurred in updatesCKOrUniqueIndexKey()
return this;
- // To be removed when TRAFODION-1610 is implemented.
- NABoolean xnsfrmHbaseUpdate = FALSE;
- if ((hbaseOper()) && (NOT isMerge()))
- {
- if (CmpCommon::getDefault(HBASE_TRANSFORM_UPDATE_TO_DELETE_INSERT) == DF_ON)
--- End diff --
I assume this is the only use of this CQD, so the CQD should be removed?
---
[GitHub] incubator-trafodion pull request #1253: [TRAFODION-1610][TRAFODION-2630] Gen...
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/1253#discussion_r142806544
--- Diff: core/sql/generator/GenRelUpdate.cpp ---
@@ -1718,19 +1806,56 @@ short HbaseUpdate::codeGen(Generator * generator)
}
else if (getIndexDesc()->isClusteringIndex() && getCheckConstraints().entries())
{
- GenAssert(FALSE, "Should not reach here. This update should have been transformed to delete/insert");
- // To be uncommented when TRAFODION-1610 is implemented
- // Need to generate insConstraintExpr also
-/*
+ // Generate the update and insert constraint check expressions
+
+ // The attributes for the columns referenced in the constraint expressions
+ // refer to the source values of the columns. We want to evaluate the
+ // constraints aganst the target values, though. So, there is some
+ // Attributes gymnastics that has to happen to generate them.
--- End diff --
Oops... I should have deleted the comments from 1811-1814
---