[GitHub] incubator-trafodion pull request #1316: [TRAFODION-2822] Make [first n] view...

2017-12-12 Thread DaveBirdsall 
Github user DaveBirdsall commented on a diff in the pull request:


https://github.com/apache/incubator-trafodion/pull/1316#discussion_r156487426
  
--- Diff: core/sql/optimizer/NormRelExpr.cpp ---
@@ -6995,11 +6995,6 @@ NABoolean RelRoot::isUpdatableBasic(NABoolean isView,
   // QSTUFF
 
 {
-  // if child is a FirstN node, skip it.
-  if ((child(0)->castToRelExpr()->getOperatorType() == REL_FIRST_N) &&
- (child(0)->child(0)))
-   scan = (Scan *)child(0)->child(0)->castToRelExpr();
-  else
scan = (Scan *)child(0)->castToRelExpr();
--- End diff --

Thanks, Suresh, for the pointer. While trying to go through the 
RelRoot::preCodeGen() logic and cause a FirstN node to be inserted, I ran into 
this bug:

1. I created a table t1 with six rows.
2. I did "create view v1 as select [first 5] * from t1 order by a;"  (I 
wanted to see if the order by clause was permitted and if so would it 
circumvent the updatable view check. It did!)
3. I did "update v1 set b = 6". It updated 6 rows.

So, it appears that the updatable view test is not as complete as it needs 
to be.

I have written a new JIRA, 
https://issues.apache.org/jira/browse/TRAFODION-2840, to track this problem.


---

[GitHub] incubator-trafodion pull request #1316: [TRAFODION-2822] Make [first n] view...

2017-12-05 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/incubator-trafodion/pull/1316


---

[GitHub] incubator-trafodion pull request #1316: [TRAFODION-2822] Make [first n] view...

2017-12-05 Thread DaveBirdsall 
Github user DaveBirdsall commented on a diff in the pull request:


https://github.com/apache/incubator-trafodion/pull/1316#discussion_r155139480
  
--- Diff: core/sql/optimizer/NormRelExpr.cpp ---
@@ -6995,11 +6995,6 @@ NABoolean RelRoot::isUpdatableBasic(NABoolean isView,
   // QSTUFF
 
 {
-  // if child is a FirstN node, skip it.
-  if ((child(0)->castToRelExpr()->getOperatorType() == REL_FIRST_N) &&
- (child(0)->child(0)))
-   scan = (Scan *)child(0)->child(0)->castToRelExpr();
-  else
scan = (Scan *)child(0)->castToRelExpr();
--- End diff --

I will look into it.


---

[GitHub] incubator-trafodion pull request #1316: [TRAFODION-2822] Make [first n] view...

2017-12-05 Thread sureshsubbiah 
Github user sureshsubbiah commented on a diff in the pull request:


https://github.com/apache/incubator-trafodion/pull/1316#discussion_r155055478
  
--- Diff: core/sql/optimizer/NormRelExpr.cpp ---
@@ -6995,11 +6995,6 @@ NABoolean RelRoot::isUpdatableBasic(NABoolean isView,
   // QSTUFF
 
 {
-  // if child is a FirstN node, skip it.
-  if ((child(0)->castToRelExpr()->getOperatorType() == REL_FIRST_N) &&
- (child(0)->child(0)))
-   scan = (Scan *)child(0)->child(0)->castToRelExpr();
-  else
scan = (Scan *)child(0)->castToRelExpr();
--- End diff --

There may be a few isolated cases where the FirstN node is added  during 
preCodeGen. Please see GenPreCode.cpp RelRoot::preCodeGen(). The example given 
there about Order by where sort is added in optimizer, or a FirstN where the N 
value is to be specified with a param seem to be cases where we would add the 
FirstN later. Will current change cause such views to marked as updateable? 
This is minor and could be resolved later, if any change is necessary.


---

[GitHub] incubator-trafodion pull request #1316: [TRAFODION-2822] Make [first n] view...

2017-11-29 Thread DaveBirdsall 
GitHub user DaveBirdsall opened a pull request:

https://github.com/apache/incubator-trafodion/pull/1316

[TRAFODION-2822] Make [first n] views non-updatable; prevent bad MERGE plans

This pull request contains two changes:

1. Views defined using [first n] or [any n] are now marked as not updatable 
and not insertable. So, when a MERGE statement is attempted on a new view of 
this type, we will get a compile time error that the view is not insertable or 
updatable (instead of getting an incorrect result).

2. MERGE statements that have a WHEN NOT MATCHED INSERT action are 
prevented from undergoing a tuple substitution transformation (that is, the 
TSJRule and TSJFlowRule will not fire on a merge node possessing a WHEN NOT 
MATCHED INSERT action). This is necessary because the run-time implementation 
of insert actions takes place in the merge node itself; that is, scanning has 
to happen in the merge node. These two rules would take the scanning out of the 
merge node into a separate scan node.

Note: Existing [first n] / [any n] views remain marked as updatable and 
insertable. (These attributes are calculated at CREATE VIEW time and stored in 
the metadata.) The second change above will cause MERGE statements having WHEN 
NOT MATCHED INSERT actions to fail at compile time against such views with an 
error 2235 (Optimizer could not produce a plan for the statement). While this 
is a non-obvious error from a user perspective it is far better than allowing 
execution and getting an incorrect result.

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

$ git pull https://github.com/DaveBirdsall/incubator-trafodion Trafodion2822

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

https://github.com/apache/incubator-trafodion/pull/1316.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 #1316






---