[TRAFODION-3034] Support Oracle Hierarchy Query (Connect By) #1688

moscowgentalman · 2018-08-10T13:33:00Z

This PR is the first patch to support Oracle Hierarchy query feature (CONNECT BY).
In this PR, the feature is implemented as a new SQL utility, It is standalone, rather clear isolated with all other SQL functions, so the impact is minimal.
In the long run, we should finish the ANSI recursive feature (recursive WITH) , and at that time, this feature can be considered to refactor to use that infrastructure.
This is just the first phase of this feature.

The basic logic is simple: the utility will run a query to get all start values (specified by the START WITH clause), then it will construct queries to search for children of the root, and loop until no children can be found.

Oracle has 3 pseudo columns, to support the ISLEAF and CONNECT_BY_PATH, the utility will have to run a query for each parent, it will be rather slow. If a query doesn't have those two pseudo columns required, the utility will run in batch mode, for each iteration, get all children in one query. That will be much faster.

One can check the executor/TEST021 for how this feature works first.

This will be a long review process, there must be many places to be modified and enhanced, thank you all for help in advance.

…N-3034 Conflicts: core/sql/generator/GenRelExeUtil.cpp core/sql/optimizer/RelExeUtil.h core/sql/sqlcomp/DefaultConstants.h

…N-3034

Traf-Jenkins · 2018-08-10T13:33:15Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2935/

moscowgentalman · 2018-08-10T13:35:46Z

core/sql/optimizer/BindRelExpr.cpp

+       }
+    }  
+   }
+#endif


oops, I will remove these dead code.

Traf-Jenkins · 2018-08-10T15:09:18Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/2935/

Traf-Jenkins · 2018-08-10T15:09:35Z

Previous Test Aborted. New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2936/

Traf-Jenkins · 2018-08-10T17:47:25Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2936/

moscowgentalman · 2018-08-15T03:20:36Z

@anoopsharma00 @zellerh @sureshsubbiah @selvaganesang could you help to take a review?

moscowgentalman · 2018-08-20T12:27:20Z

I need a big rework of this patch. Please hold until I finish.
The current implemention takes assumption there is only one table in the from_clause, which is not correct.

…JOIN

Traf-Jenkins · 2018-08-25T02:39:13Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2961/

Traf-Jenkins · 2018-08-25T05:15:36Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/2961/

Traf-Jenkins · 2018-08-25T12:57:10Z

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2962/

Traf-Jenkins · 2018-08-25T15:28:37Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/2962/

moscowgentalman · 2018-08-26T02:25:14Z

jenkins, retest

Traf-Jenkins · 2018-08-26T02:27:12Z

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2965/

Traf-Jenkins · 2018-08-26T05:01:53Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/2965/

Traf-Jenkins · 2018-08-26T14:57:10Z

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2966/

Traf-Jenkins · 2018-08-26T17:34:22Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2966/

robertamarton · 2018-09-26T23:26:31Z

What are the security considerations for this features? Do standard grant privileges work find without changes? Could one possible example for using CONNECT BY it to traverse the privilege tree. Start with the grantor being the system and proceed to through the children (where grantee == grantor).

sureshsubbiah

I am about 30% done with the review I think. It will likely take me another 2-3 days to complete it. I like the coding style and content very much. It is clean, easy to understand and flows well.

sureshsubbiah · 2018-10-01T03:30:19Z

core/sql/bin/SqlciErrors.txt

@@ -1515,6 +1515,9 @@ $1~String1 --------------------------------
 8034 ZZZZZ 99999 ADVANCED MAJOR DBADMIN Column $0~String0 of object $1~string1 does not have a default clause but it is missing in database. This indicates inconsistent data.
 8035 ZZZZZ 99999 ADVANCED MAJOR DBADMIN Truncation of hive table failed. $0~String0
 8036 ZZZZZ 99999 ADVANCED MINOR LOGONLY Error while creating the error logging file or logging the error row to file $0~String0: Details :$1~String1 
+8037 ZZZZZ 99999 BEGINNER MAJOR DBADMIN Loop detected in connect by execution.


In these three messages, it would help if we could give information specific to the instance when the error occurred. Maybe value of the prior column(s) as the error occurred. For the case of recursion exceeding a certain length or memory being exhausted, more details what level of recursion has been reached or how much memory has been consumed (if that information is readily available) would be helpful. This is an advisory suggestion, and by no means something to be addressed soon.

I would have expected some binder errors for statements where hierarchical constructs are not supported. It is possible though unlikely that all are caught in the parser. For example what happens if a hierarchical function or pseudocolumn is used a regular (non-hierarchical query)

sureshsubbiah · 2018-10-01T03:32:37Z

core/sql/comexe/ComTdbExeUtil.cpp

+{
+   setNodeType(ComTdb::ex_CONNECT_BY);
+   connTableName_ = tableName;
+   maxDeep_ = 200; //by default, max deep of a tree


These two literals could be consts to improve readability. It looks like there are cqds defined to make these configurable, which is excellent.

sureshsubbiah · 2018-10-01T05:06:48Z

core/sql/parser/ParKeyWords.cpp

@@ -862,7 +863,7 @@ ParKeyWord ParKeyWords::keyWords_[] = {
  ParKeyWord("PREPARE",            TOK_PREPARE,     ANS_|RESWORD_),
  ParKeyWord("PRESERVE",           TOK_PRESERVE,    ANS_|RESWORD_),
  ParKeyWord("PRIMARY",            TOK_PRIMARY,     FIRST_|ANS_|RESWORD_),
-  ParKeyWord("PRIOR",              IDENTIFIER,      ANS_|RESWORD_),
+  ParKeyWord("PRIOR",              PRIOR_IDENTIFIER,      ANS_),


Could you please explain why this is a PRIOR_IDENTIFIER and not TOK_PRIOR? Are we attempting to allow use of PRIOR as an identifier now. It was previously not allowed, since it was a RESWORD. Are we trying to have fewer reserved words? This would be an admirable goal. Thank you for explaining, this is not a defect.

sureshsubbiah · 2018-10-02T03:18:55Z

core/sql/parser/sqlparser.y

@@ -13280,13 +13288,130 @@ table_expression : from_clause where_clause sample_clause
 		                                 SqlParser_CurrentParser->topHasOlapFunctions());
                     SqlParser_CurrentParser->setTopHasTDFunctions(FALSE);
 		   }
+            | from_clause startwith_clause where_clause 
+                   {
+                     if($1->getOperatorType() == REL_JOIN)


I am confused on several aspects here. None maybe issues

In line 13297 the WHERE clause is passed but not in line 13310. IF branch is taken if we have a Join. Why would the WHERE clause not be passed in for a single scan? Is it because it is already addressed in the preceding rule. If yes, what benefit do lines 13308 to 13318 provide?

Are OLAP functions in the select list supported? Should sequence functions be supported too (since they are similar)?

I wonder if this whole block of code could be expressed more concisely. There seems to be some redundancy now.

sureshsubbiah · 2018-10-02T03:31:36Z

core/sql/parser/sqlparser.y

 /* type relx */
 from_clause : TOK_FROM global_hint table_reference { $$ = $3; }
 	    | from_clause ',' table_reference
 			      {
 				$$ = new (PARSERHEAP()) Join($1, $3, REL_JOIN);
-			      }
+		      }
+startwith_clause :TOK_START_WITH search_condition CONNECT_IDENTIFIER TOK_BY search_condition


From Oracle doc "In a hierarchical query, one expression in condition must be qualified with the PRIOR operator to refer to the parent row". Is this check done later?

sureshsubbiah · 2018-10-02T03:56:27Z

core/sql/optimizer/RelExpr.h

@@ -1439,6 +1459,7 @@ class RelExpr : public ExprNode
  enum Flags {
    EXPAND_SHORT_ROWS  = 0x00000001     // expand short rows when added columns
   ,PARENT_IS_ROOT     = 0x00000002     // compressed internal format
+   ,HAS_CONNECT_BY     = 0x00000004     // compressed internal format


) typo on the comment.
2) why do we need a bit flag on the RelExpr? Can we just check nullness of biConnectBy and other data members of this class? I was under the impression that we did not need bit flags in RelExpr data members in the compiler till be reached the generator. This could be an incorrect idea.
3) Can any RelExpr have a CONNECT_BY. I am assuming there are several RelExpr where this is not allowed/supported. We should add binder error for those that are not caught in the parser, if any.

sureshsubbiah · 2018-10-02T04:10:02Z

core/sql/optimizer/ValueDesc.cpp

@@ -4555,7 +4555,7 @@ void ValueIdSet::unparse(NAString &result,

  NAString connectorText;

-  if ((form == MVINFO_FORMAT) || (form == QUERY_FORMAT))
+  if ((form == MVINFO_FORMAT) || (form == QUERY_FORMAT) || (form == CONNECT_BY_FORMAT))


How is the CONNECT_BY_FORMAT different from QUERY or MVINFO formats? The answer might be in other parts of code, its just that I cannot tell. Thank you for explaining

CONNECT_BY_FORMAT has only one difference from QUERY_FORMAT, that the column name only has the last part, not qualified.
The reason is: the column name will be used in the work() method of CONNECT_BY, where alias of the table name is lost. But we can make sure, there is only one table in that query in the work() method, so only the column name is required.

sureshsubbiah · 2018-10-02T04:12:08Z

core/sql/optimizer/RelExpr.h

@@ -525,7 +540,7 @@ class RelExpr : public ExprNode
  // QSTUFF

  // --------------------------------------------------------------------
-  // normalizeNode() performs predicate pushdown and also ensures
+  // normalizeNode() performs predicate pushdown and also ensuresL


sureshsubbiah · 2018-10-02T04:22:24Z

core/sql/executor/ExExeUtil.h

@@ -4138,6 +4138,7 @@ class ExExeUtilLobInfoTableTcb : public ExExeUtilTcb
    DONE_
  };
  Step step_;
+  char * data_;


Why do we need a change in a Lob class?

sureshsubbiah · 2018-10-02T04:52:10Z

core/sql/executor/ExExeUtil.h

@@ -4172,6 +4173,48 @@ class ExExeUtilLobInfoPrivateState : public ex_tcb_private_state
 protected:
 };

+class connectByStackItem


I think it is unusual in the executor to have classes that do not have a base class. How about memory management for this class. I might understand more as I see how these classes are used.

moscowgentalman added 15 commits June 1, 2018 22:44

[TRAFODION-3034] add parser changes

5fb8f21

basic work finish

b29f4dd

fix parser, add loop detection

88e8b20

refactor the code, next need to add CQD and where clause support

9fb3a59

support where clause, next add CQD to control memory usage

b875ec3

support where clause, next add regression test

1b853c5

support both where clause and order by, next add test cases

f20b63c

add test case, next to rebase and try to support is_cycle

5757f25

Merge branch 'master' of git://git.apache.org/trafodion into TRAFODIO…

73a175a

…N-3034 Conflicts: core/sql/generator/GenRelExeUtil.cpp core/sql/optimizer/RelExeUtil.h core/sql/sqlcomp/DefaultConstants.h

support PATH, next try to support is leaf

1b32b6b

add ISLEAF support, but need to wait for another enhancement

81ef30d

finish is leaf

d781d88

Merge branch 'master' of git://git.apache.org/trafodion into TRAFODIO…

cc9057d

…N-3034

first phase finished

8ae2ed9

fix regression test case and remove some dead code

26d9b06

moscowgentalman commented Aug 10, 2018

View reviewed changes

core/sql/optimizer/BindRelExpr.cpp Outdated

}

}

}

#endif

Copy link

Contributor Author

moscowgentalman Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, I will remove these dead code.

handle the null value

6dd9c8e

rework the compiler part, move the processing into binder to support …

d6fb419

…JOIN

add support for order by siblyings

e1a5911

fix reduce/shift conflict

caed495

sureshsubbiah reviewed Oct 2, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TRAFODION-3034] Support Oracle Hierarchy Query (Connect By) #1688

[TRAFODION-3034] Support Oracle Hierarchy Query (Connect By) #1688

moscowgentalman commented Aug 10, 2018

Traf-Jenkins commented Aug 10, 2018

moscowgentalman Aug 10, 2018

Traf-Jenkins commented Aug 10, 2018

Traf-Jenkins commented Aug 10, 2018

Traf-Jenkins commented Aug 10, 2018

moscowgentalman commented Aug 15, 2018

moscowgentalman commented Aug 20, 2018

Traf-Jenkins commented Aug 25, 2018

Traf-Jenkins commented Aug 25, 2018

Traf-Jenkins commented Aug 25, 2018

Traf-Jenkins commented Aug 25, 2018

moscowgentalman commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

robertamarton commented Sep 26, 2018

sureshsubbiah left a comment

sureshsubbiah Oct 1, 2018

sureshsubbiah Oct 1, 2018

sureshsubbiah Oct 1, 2018

sureshsubbiah Oct 2, 2018

sureshsubbiah Oct 2, 2018

sureshsubbiah Oct 2, 2018

sureshsubbiah Oct 2, 2018

moscowgentalman Oct 6, 2018

sureshsubbiah Oct 2, 2018

sureshsubbiah Oct 2, 2018

sureshsubbiah Oct 2, 2018

[TRAFODION-3034] Support Oracle Hierarchy Query (Connect By) #1688

Are you sure you want to change the base?

[TRAFODION-3034] Support Oracle Hierarchy Query (Connect By) #1688

Conversation

moscowgentalman commented Aug 10, 2018

Traf-Jenkins commented Aug 10, 2018

Choose a reason for hiding this comment

Traf-Jenkins commented Aug 10, 2018

Traf-Jenkins commented Aug 10, 2018

Traf-Jenkins commented Aug 10, 2018

moscowgentalman commented Aug 15, 2018

moscowgentalman commented Aug 20, 2018

Traf-Jenkins commented Aug 25, 2018

Traf-Jenkins commented Aug 25, 2018

Traf-Jenkins commented Aug 25, 2018

Traf-Jenkins commented Aug 25, 2018

moscowgentalman commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

Traf-Jenkins commented Aug 26, 2018

robertamarton commented Sep 26, 2018

sureshsubbiah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment