You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by traflm <gi...@git.apache.org> on 2018/03/19 03:23:07 UTC
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
GitHub user traflm opened a pull request:
https://github.com/apache/trafodion/pull/1481
[Trafodion-2335] support functions in the column default definition
This PR add the support in column default definition the expansion to use following functions:
uuid
unix_timestamp
to_char
To support to_char, it saves the original text of full to_char function, and save it as DEFAULT_VALUE in "_MD_".COLUMNS.
And the INSERT binder will get this original string, and parse into the ItemExpr tree.
The semantic check is done in setDefaultAttribute.
UUID/UNIX_TIMESTAMP follow the current_timestamp implementation.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/traflm/trafodion TRAFODION-2335
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1481.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 #1481
----
commit 6b800c34a4f9cb4386223067ff7c3801bdbec002
Author: Liu Ming <ov...@...>
Date: 2018-03-14T09:07:42Z
[TRAFODION-2335] support some functions as column default, part1 commit
commit 5f58cd95aa72f4cbe944aa8d0aeb11eb0202f524
Author: Liu Ming <ov...@...>
Date: 2018-03-18T09:19:35Z
update the regression test and various fixes
----
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r176114627
--- Diff: core/sql/common/ComSmallDefs.h ---
@@ -609,47 +609,59 @@ enum ComColumnClass { COM_UNKNOWN_CLASS
#define COM_ALTERED_USER_COLUMN_LIT "C "
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT
+ , COM_CURRENT_UT_DEFAULT
--- End diff --
Were you able to do the development/testing of this change without re-initializing trafodion in your work space? I ran the following query in the metadata in my work space. I believe the enum value is being stored, If so, this change can cause incompatibility. Jenkins tests always initialize trafodion.
>>select distinct default_class from "_MD_".columns ;
DEFAULT_CLASS
-------------
1
3
2
5
7
6
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r176177437
--- Diff: core/sql/common/ComSmallDefs.h ---
@@ -609,47 +609,59 @@ enum ComColumnClass { COM_UNKNOWN_CLASS
#define COM_ALTERED_USER_COLUMN_LIT "C "
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT
+ , COM_CURRENT_UT_DEFAULT
--- End diff --
I'm glad you found this. I would recommend to add a big comment saying that these values are stored in the metadata (COLUMNS.DEFAULT_CLASS) and cannot be changed (this is in addition to actually assigning the numbers, as Anoop said). There are only few such enums and it would be good if those were very clearly marked.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r179320050
--- Diff: core/sql/regress/seabase/EXPECTED020 ---
@@ -3945,4 +3945,35 @@ AAAA ? 2
--- 1 row(s) selected.
>>
+>>obey TEST020(trafodion_2335);
+>>--create seqence test020_seq;
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b char(36) not null default uuid(),
++> c varchar(10) default to_char(sysdate,'YYYYMMDD'),
++> --support sequence as default in next check-in
++> --d int not null default testi020_seq.nextval,
++> e int );
+
+--- SQL operation complete.
+>>insert into test020t45(e) values(1),(2),(3);
+
+--- 3 row(s) inserted.
+>>select count(*) from test020t45 where c = to_char(sysdate,'YYYYMMDD');
+
+(EXPR)
+--------------------
+
+ 3
+
+--- 1 row(s) selected.
+>>--negative tests
+>>--the function is not variable-free, so should fail
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b varchar(10) default to_char(test020t45.c,'YYYYMMDD'),
++> e int );
+
+*** ERROR[1084] An invalid default value was specified for column B.
+
+*** ERROR[8822] The statement was not prepared.
+
--- End diff --
Please consider doing the same tests for aligned format table in a later check-in
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r175861985
--- Diff: core/sql/common/ComSmallDefs.h ---
@@ -609,47 +609,59 @@ enum ComColumnClass { COM_UNKNOWN_CLASS
#define COM_ALTERED_USER_COLUMN_LIT "C "
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT
+ , COM_CURRENT_UT_DEFAULT
--- End diff --
I wonder if this can cause incompatibility with existing metadata if this value is stored in metadata
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r176279153
--- Diff: core/sql/common/ComSmallDefs.h ---
@@ -609,47 +609,59 @@ enum ComColumnClass { COM_UNKNOWN_CLASS
#define COM_ALTERED_USER_COLUMN_LIT "C "
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT
+ , COM_CURRENT_UT_DEFAULT
--- End diff --
Hi Ming, yes, you are right, I meant that the existing numbers cannot be changed. Adding new numbers is ok.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r175651051
--- Diff: core/sql/parser/ElemDDLColDef.h ---
@@ -309,6 +312,8 @@ class ElemDDLColDef : public ElemDDLNode
NABoolean seabaseSerialized_;
NABoolean isColDefaultSpec_;
+
+ Int32 errCode_;
--- End diff --
This is leftover code, I will remove it. When there are more comments, and change together
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r176618285
--- Diff: core/sql/regress/seabase/EXPECTED020 ---
@@ -3945,4 +3945,35 @@ AAAA ? 2
--- 1 row(s) selected.
>>
+>>obey TEST020(trafodion_2335);
+>>--create seqence test020_seq;
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b char(36) not null default uuid(),
++> c varchar(10) default to_char(sysdate,'YYYYMMDD'),
++> --support sequence as default in next check-in
++> --d int not null default testi020_seq.nextval,
++> e int );
+
+--- SQL operation complete.
+>>insert into test020t45(e) values(1),(2),(3);
+
+--- 3 row(s) inserted.
+>>select count(*) from test020t45 where c = to_char(sysdate,'YYYYMMDD');
+
+(EXPR)
+--------------------
+
+ 3
+
+--- 1 row(s) selected.
+>>--negative tests
+>>--the function is not variable-free, so should fail
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b varchar(10) default to_char(test020t45.c,'YYYYMMDD'),
++> e int );
+
+*** ERROR[1084] An invalid default value was specified for column B.
+
+*** ERROR[8822] The statement was not prepared.
+
--- End diff --
@selvaganesang By adding new test case for upsert mode 'merge', the missing column of unix_timestamp() default value is updated. So still something wrong, I need to fix it. Thanks for reminding this.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r176278679
--- Diff: core/sql/common/ComSmallDefs.h ---
@@ -609,47 +609,59 @@ enum ComColumnClass { COM_UNKNOWN_CLASS
#define COM_ALTERED_USER_COLUMN_LIT "C "
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT
+ , COM_CURRENT_UT_DEFAULT
--- End diff --
Thanks all, I now understand what is the issue! If there are old metadata, and there is a upgrade, then things are messed up.
But I think this enum can be changed by adding new value at the end, instead of "cannot be changed". So let me add them at the end, and add big comments.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r176110128
--- Diff: core/sql/regress/seabase/EXPECTED020 ---
@@ -3945,4 +3945,35 @@ AAAA ? 2
--- 1 row(s) selected.
>>
+>>obey TEST020(trafodion_2335);
+>>--create seqence test020_seq;
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b char(36) not null default uuid(),
++> c varchar(10) default to_char(sysdate,'YYYYMMDD'),
++> --support sequence as default in next check-in
++> --d int not null default testi020_seq.nextval,
++> e int );
+
+--- SQL operation complete.
+>>insert into test020t45(e) values(1),(2),(3);
+
+--- 3 row(s) inserted.
+>>select count(*) from test020t45 where c = to_char(sysdate,'YYYYMMDD');
+
+(EXPR)
+--------------------
+
+ 3
+
+--- 1 row(s) selected.
+>>--negative tests
+>>--the function is not variable-free, so should fail
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b varchar(10) default to_char(test020t45.c,'YYYYMMDD'),
++> e int );
+
+*** ERROR[1084] An invalid default value was specified for column B.
+
+*** ERROR[8822] The statement was not prepared.
+
--- End diff --
@traflm Thank for adding the test for upsert. This test just ensures that the upsert command worked. But, it doesn't check if the added default values are correct. You can take some cue from earlier tests in the same test file seabase/TEST020. Search for cqd traf_upsert_mode
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r175876149
--- Diff: core/sql/regress/seabase/EXPECTED020 ---
@@ -3945,4 +3945,35 @@ AAAA ? 2
--- 1 row(s) selected.
>>
+>>obey TEST020(trafodion_2335);
+>>--create seqence test020_seq;
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b char(36) not null default uuid(),
++> c varchar(10) default to_char(sysdate,'YYYYMMDD'),
++> --support sequence as default in next check-in
++> --d int not null default testi020_seq.nextval,
++> e int );
+
+--- SQL operation complete.
+>>insert into test020t45(e) values(1),(2),(3);
+
+--- 3 row(s) inserted.
+>>select count(*) from test020t45 where c = to_char(sysdate,'YYYYMMDD');
+
+(EXPR)
+--------------------
+
+ 3
+
+--- 1 row(s) selected.
+>>--negative tests
+>>--the function is not variable-free, so should fail
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b varchar(10) default to_char(test020t45.c,'YYYYMMDD'),
++> e int );
+
+*** ERROR[1084] An invalid default value was specified for column B.
+
+*** ERROR[8822] The statement was not prepared.
+
--- End diff --
Can you please add tests to test upsert with default function definition when these columns are omitted in upsert command. Upsert can be transformed into merge or plain upsert. It is based on the TRAF_UPSERT_MODE.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r175641075
--- Diff: core/sql/parser/sqlparser.y ---
@@ -8734,6 +8738,30 @@ datetime_value_function : TOK_CURDATE '(' ')'
$$ = new (PARSERHEAP()) Cast(uniqueId,type);
}
+/* type item */
+datetime_misc_function_used_as_default: TOK_TO_CHAR '(' value_expression ',' character_string_literal ')'
+ {
+ NAString * ves= unicodeToChar
+ (ToTokvalPlusYYText(&$3)->yytext,
+ ToTokvalPlusYYText(&$3)->yyleng,
+ (CharInfo::CharSet) (
+ ComGetNameInterfaceCharSet() // CharInfo::UTF8
+ ),
+ PARSERHEAP());
+ //save the original text
+ NAString fullstr;
+ fullstr += "TO_CHAR(";
--- End diff --
to_char(sysdate, 'YYYYMMDD') unparse to
TO_CHAR(CAST(currrenttimestmp), 'YYYYMMDD')
Which cannot be compiled later. Given more time, I may be able to find the right place to modify the unparse for sysdate, but the CAST is something that very hard to make it generic. Other unparse usage place may need the CAST. So for now, I have to keep the original text in this way.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r175964689
--- Diff: core/sql/common/ComSmallDefs.h ---
@@ -609,47 +609,59 @@ enum ComColumnClass { COM_UNKNOWN_CLASS
#define COM_ALTERED_USER_COLUMN_LIT "C "
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT
+ , COM_CURRENT_UT_DEFAULT
--- End diff --
@robertamarton Could you help to provide some comments here?
@selvaganesang I think new values will NOT cause problems, the MD table structure is not changed. I don't find any checking code relying on values in this column. But Roberta should have more insight here.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r175966341
--- Diff: core/sql/regress/seabase/EXPECTED020 ---
@@ -3945,4 +3945,35 @@ AAAA ? 2
--- 1 row(s) selected.
>>
+>>obey TEST020(trafodion_2335);
+>>--create seqence test020_seq;
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b char(36) not null default uuid(),
++> c varchar(10) default to_char(sysdate,'YYYYMMDD'),
++> --support sequence as default in next check-in
++> --d int not null default testi020_seq.nextval,
++> e int );
+
+--- SQL operation complete.
+>>insert into test020t45(e) values(1),(2),(3);
+
+--- 3 row(s) inserted.
+>>select count(*) from test020t45 where c = to_char(sysdate,'YYYYMMDD');
+
+(EXPR)
+--------------------
+
+ 3
+
+--- 1 row(s) selected.
+>>--negative tests
+>>--the function is not variable-free, so should fail
+>>create table test020t45(a largeint not null default unix_timestamp(),
++> b varchar(10) default to_char(test020t45.c,'YYYYMMDD'),
++> e int );
+
+*** ERROR[1084] An invalid default value was specified for column B.
+
+*** ERROR[8822] The statement was not prepared.
+
--- End diff --
Thanks Selva, Let me add such a test case.
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by anoopsharma00 <gi...@git.apache.org>.
Github user anoopsharma00 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r176166576
--- Diff: core/sql/common/ComSmallDefs.h ---
@@ -609,47 +609,59 @@ enum ComColumnClass { COM_UNKNOWN_CLASS
#define COM_ALTERED_USER_COLUMN_LIT "C "
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT
+ , COM_CURRENT_UT_DEFAULT
--- End diff --
that is a good point. If a new value is being added to an existing enum in common/ComSmallDefs.h and that value will be stored in metadata, then it cannot be added in the middle of that enum. It should be added at the end.
Better yet, assign a number to the enums so they dont cause an issue if a new
value is added in the middle.
Something like:
enum ComColumnDefaultClass { COM_CURRENT_DEFAULT = 0
, COM_NO_DEFAULT = 1
, COM_NULL_DEFAULT = 2
, COM_USER_DEFINED_DEFAULT = 3
, COM_USER_FUNCTION_DEFAULT = 4
, COM_IDENTITY_GENERATED_BY_DEFAULT = 5
, COM_IDENTITY_GENERATED_ALWAYS = 6
, COM_ALWAYS_COMPUTE_COMPUTED_COLUMN_DEFAULT = 7
, COM_ALWAYS_DEFAULT_COMPUTED_COLUMN_DEFAULT = 8
};
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1481
---
[GitHub] trafodion pull request #1481: [Trafodion-2335] support functions in the colu...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1481#discussion_r175629733
--- Diff: core/sql/parser/sqlparser.y ---
@@ -8734,6 +8738,30 @@ datetime_value_function : TOK_CURDATE '(' ')'
$$ = new (PARSERHEAP()) Cast(uniqueId,type);
}
+/* type item */
+datetime_misc_function_used_as_default: TOK_TO_CHAR '(' value_expression ',' character_string_literal ')'
+ {
+ NAString * ves= unicodeToChar
+ (ToTokvalPlusYYText(&$3)->yytext,
+ ToTokvalPlusYYText(&$3)->yyleng,
+ (CharInfo::CharSet) (
+ ComGetNameInterfaceCharSet() // CharInfo::UTF8
+ ),
+ PARSERHEAP());
+ //save the original text
+ NAString fullstr;
+ fullstr += "TO_CHAR(";
--- End diff --
I'm curious why you needed to save the original text. Was the unparse function on the DateFormat node not adequate?
---