You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by kakaxi3019 <gi...@git.apache.org> on 2018/06/26 05:55:55 UTC
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
GitHub user kakaxi3019 opened a pull request:
https://github.com/apache/trafodion/pull/1622
[TRAFODION-3118] Improve on parts of EXTRACT
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kakaxi3019/trafodion trafodion-3118
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1622.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 #1622
----
commit b90abcebc664aa27968c9378c91b416abc71eea4
Author: kakaxi3019 <ce...@...>
Date: 2018-06-25T09:09:16Z
trafodion-3118 Improve on parts of EXTRACT
commit fc8873cae019fc4f2125da9779687ebdd31a5964
Author: kakaxi3019 <ju...@...>
Date: 2018-06-26T05:53:15Z
fix yacc conflicts
----
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r199649187
--- Diff: core/sql/parser/sqlparser.y ---
@@ -500,6 +501,7 @@ static void enableMakeQuotedStringISO88591Mechanism()
%token <tokval> TOK_CARDINALITY
%token <tokval> TOK_CASE
%token <tokval> TOK_CAST
+%token <tokval> TOK_CENTURY
--- End diff --
The new tokens, TOK_CENTURY, TOK_DECADE, etc., need to be added to the nonreserved_word production, otherwise they will be taken as reserved words (notwithstanding the NONRESTOKEN_ flag in ParKeyWords.cpp). Please also add them to test core/TEST037, which tests that non-reserved words really are non-reserved.
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by kakaxi3019 <gi...@git.apache.org>.
Github user kakaxi3019 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r200242748
--- Diff: core/sql/optimizer/SynthType.cpp ---
@@ -4470,15 +4489,31 @@ const NAType *Extract::synthesizeType()
else if (getExtractField() == REC_DATE_YEARWEEK_EXTRACT ||
getExtractField() == REC_DATE_YEARWEEK_D_EXTRACT)
prec = 6; // YEARMWEEK is yyyyww
+ else if (getExtractField() == REC_DATE_DECADE ||
+ getExtractField() == REC_DATE_DOY)
+ prec = 3;
+ else if (getExtractField() == REC_DATE_QUARTER ||
+ getExtractField() == REC_DATE_DOW)
+ prec = 1;
+ else if (getExtractField() == REC_DATE_EPOCH)
+ prec = 10;
else
prec = 2; // else max of 12, 31, 24, 59
if (getExtractField() == REC_DATE_SECOND) {
prec += dti.getFractionPrecision();
scale += dti.getFractionPrecision();
}
+ if (getExtractField() == REC_DATE_EPOCH)
+ {
+ prec += dti.getFractionPrecision();
+ scale += dti.getFractionPrecision();
+ }
+ NABoolean bNegValue = FALSE;
+ if ( getExtractField() >= REC_DATE_CENTURY && extractStartField <= REC_DATE_WOM )
+ bNegValue = TRUE;
--- End diff --
Thanks Dave, there are some problems here.
options DECADE,QUARTER,EPOCH can be negative values
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by kakaxi3019 <gi...@git.apache.org>.
Github user kakaxi3019 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r200241684
--- Diff: core/sql/regress/seabase/TEST030 ---
@@ -98,6 +98,11 @@ select HOUR(interval '5:2:15:36.33' day to second(2)) from (values(1)) as t(a);
select MINUTE(interval '5:13:25:2.12' day to second(2)) from (values(1)) as t(a);
select extract (year from INTERVAL '97-02' YEAR TO MONTH) from (values (1)) as t(a);
select interval '8' year / 4 from dual;
+select extract(week from date '2005-01-01') from (values(1)) as t(a);
+select extract(epoch from timestamp '2000-12-30 20:38:40.12') from (values(1)) as t(a);
+select extract(dow from timestamp '2018-06-21 20:38:40') from (values(1)) as t(a);
+select extract(doy from timestamp '2018-06-21 20:38:40') from (values(1)) as t(a);
+select extract(wom from timestamp '2018-06-21 20:38:40') from (values(1)) as t(a);
--- End diff --
add century,decade,quarter tests
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1622
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r199650319
--- Diff: core/sql/optimizer/SynthType.cpp ---
@@ -4416,6 +4416,22 @@ const NAType *Extract::synthesizeType()
return NULL;
}
+ if ( type != NA_DATETIME_TYPE )
+ {
+ enum rec_datetime_field eField = getExtractField();
+ NAString sErr;
+ if ( REC_DATE_WEEK == eField
+ || REC_DATE_DOW == eField
+ || REC_DATE_DOY == eField
+ || REC_DATE_WOM == eField
+ || REC_DATE_CENTURY == eField)
--- End diff --
Should DECADE be in this list too?
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by kakaxi3019 <gi...@git.apache.org>.
Github user kakaxi3019 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r200241409
--- Diff: core/sql/parser/sqlparser.y ---
@@ -500,6 +501,7 @@ static void enableMakeQuotedStringISO88591Mechanism()
%token <tokval> TOK_CARDINALITY
%token <tokval> TOK_CASE
%token <tokval> TOK_CAST
+%token <tokval> TOK_CENTURY
--- End diff --
add TOK_CENTURY, TOK_DECADE, TOK_DOW, TOK_DOY ,TOK_EPOCH, TOK_WOM to nonreserved_word
TOK_WEEK, TOK_QUARTER already exists in nonreserved_func_word
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r199649960
--- Diff: core/sql/regress/seabase/TEST030 ---
@@ -98,6 +98,11 @@ select HOUR(interval '5:2:15:36.33' day to second(2)) from (values(1)) as t(a);
select MINUTE(interval '5:13:25:2.12' day to second(2)) from (values(1)) as t(a);
select extract (year from INTERVAL '97-02' YEAR TO MONTH) from (values (1)) as t(a);
select interval '8' year / 4 from dual;
+select extract(week from date '2005-01-01') from (values(1)) as t(a);
+select extract(epoch from timestamp '2000-12-30 20:38:40.12') from (values(1)) as t(a);
+select extract(dow from timestamp '2018-06-21 20:38:40') from (values(1)) as t(a);
+select extract(doy from timestamp '2018-06-21 20:38:40') from (values(1)) as t(a);
+select extract(wom from timestamp '2018-06-21 20:38:40') from (values(1)) as t(a);
--- End diff --
Consider adding tests for the other options, e.g. CENTURY, DECADE
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by kakaxi3019 <gi...@git.apache.org>.
Github user kakaxi3019 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r200242167
--- Diff: core/sql/optimizer/SynthType.cpp ---
@@ -4416,6 +4416,22 @@ const NAType *Extract::synthesizeType()
return NULL;
}
+ if ( type != NA_DATETIME_TYPE )
+ {
+ enum rec_datetime_field eField = getExtractField();
+ NAString sErr;
+ if ( REC_DATE_WEEK == eField
+ || REC_DATE_DOW == eField
+ || REC_DATE_DOY == eField
+ || REC_DATE_WOM == eField
+ || REC_DATE_CENTURY == eField)
--- End diff --
DECADE,EPOCH,QUARTER should not be in this list, because DECADE support interval.
exp.
EXTRACT(DECADE FROM INTERVAL '9' YEAR - INTERVAL '99' YEAR);
---
[GitHub] trafodion pull request #1622: [TRAFODION-3118] Improve on parts of EXTRACT
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1622#discussion_r199651226
--- Diff: core/sql/optimizer/SynthType.cpp ---
@@ -4470,15 +4489,31 @@ const NAType *Extract::synthesizeType()
else if (getExtractField() == REC_DATE_YEARWEEK_EXTRACT ||
getExtractField() == REC_DATE_YEARWEEK_D_EXTRACT)
prec = 6; // YEARMWEEK is yyyyww
+ else if (getExtractField() == REC_DATE_DECADE ||
+ getExtractField() == REC_DATE_DOY)
+ prec = 3;
+ else if (getExtractField() == REC_DATE_QUARTER ||
+ getExtractField() == REC_DATE_DOW)
+ prec = 1;
+ else if (getExtractField() == REC_DATE_EPOCH)
+ prec = 10;
else
prec = 2; // else max of 12, 31, 24, 59
if (getExtractField() == REC_DATE_SECOND) {
prec += dti.getFractionPrecision();
scale += dti.getFractionPrecision();
}
+ if (getExtractField() == REC_DATE_EPOCH)
+ {
+ prec += dti.getFractionPrecision();
+ scale += dti.getFractionPrecision();
+ }
+ NABoolean bNegValue = FALSE;
+ if ( getExtractField() >= REC_DATE_CENTURY && extractStartField <= REC_DATE_WOM )
+ bNegValue = TRUE;
--- End diff --
I'm confused. From this code, it looks like all of the new options can yield negative values. Yet, earlier, for example, QUARTER is limited to a precision of 1. I could imagine QUARTER being negative if it were with respect to the first quarter of year 1 AD. But since the precision is 1, I'm guessing that QUARTER is always with respect to the current year, so it only takes the values 1, 2, 3 or 4. I'm wondering what DECADE, DOM, DOY and WOM are; what are they relative to? And can they be negative?
---