You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by jinossy <gi...@git.apache.org> on 2016/04/04 10:59:30 UTC
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
GitHub user jinossy opened a pull request:
https://github.com/apache/tajo/pull/993
TAJO-2110: Fix incorrect DateTime and remove hard coded tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jinossy/tajo TAJO-2110
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/993.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 #993
----
commit 6f3d71c4cff95ffe1a7d14804c85baaf11606f81
Author: Jinho Kim <jh...@apache.org>
Date: 2016-04-04T08:58:16Z
TAJO-2110: Fix incorrect DateTime and remove hard coded tests.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60342924
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/datetime/AddDays.java ---
@@ -52,8 +56,16 @@ public AddDays() {
}
@Override
+ public void init(OverridableConf context, FunctionEval.ParamType[] types) {
+ if (!hasTimeZone()) {
+ setTimeZone(context.getConf().getSystemTimezone());
--- End diff --
Sorry, I forgot to mention that I changed the client timezone.
The original data is
```
default> select * from test;
t, ts, d
-------------------------------
01:01:23, 2015-05-01 15:01:23, 2015-05-01
02:02:33, 2016-05-01 16:02:33, 2016-05-01
```
, and the above case can be reproduced as follows.
```
default> \set TIMEZONE GMT-5
default> select * from test;
t, ts, d
-------------------------------
01:01:23, 2015-05-01 01:01:23, 2015-05-01
02:02:33, 2016-05-01 02:02:33, 2016-05-01
(2 rows, 0.018 sec, 0 B selected)
default> select add_days(ts, 5) from test;
?add_days
-------------------------------
2015-05-06 01:01:23
2016-05-06 02:02:33
(2 rows, 0.171 sec, 0 B selected)
default> select add_days(d, 5) from test;
?add_days
-------------------------------
2015-05-05 10:00:00
2016-05-05 10:00:00
```
The result of add_days() looks to be affected by the client timezone. This might be intended behavior because the type of the result is timestamp, but it seems weird. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60339699
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/datetime/AddDays.java ---
@@ -52,8 +56,16 @@ public AddDays() {
}
@Override
+ public void init(OverridableConf context, FunctionEval.ParamType[] types) {
+ if (!hasTimeZone()) {
+ setTimeZone(context.getConf().getSystemTimezone());
--- End diff --
```add_days()``` and ```add_months()``` seem to be separately implemented for each type of ```date``` and ```timestamp```. Here is a case which looks a bug. The results of ```add_days()``` of timestamp and date types are expected to be same, but is different.
```
default> \d test
table name: default.test
table uri: hdfs://localhost:7020/tajo/warehouse/default/test
store type: TEXT
number of rows: 1
volume: 40 B
Options:
'timezone'='Asia/Seoul'
'text.delimiter'='|'
schema:
t TIME
ts TIMESTAMP
d DATE
default> select * from test;
t, ts, d
-------------------------------
01:01:23, 2015-05-01 01:01:23, 2015-05-01
02:02:33, 2016-05-01 02:02:33, 2016-05-01
(2 rows, 0.019 sec, 0 B selected)
default> select add_days(ts, 5) from test;
?add_days
-------------------------------
2015-05-06 01:01:23
2016-05-06 02:02:33
(2 rows, 0.026 sec, 0 B selected)
default> select add_days(d, 5) from test;
?add_days
-------------------------------
2015-05-05 10:00:00
2016-05-05 10:00:00
(2 rows, 0.023 sec, 0 B selected)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-211822913
Hi @jinossy, thanks for work. This is a really critical bug.
Your patch looks good, but I found some issues.
* Please disable the addition and subtraction of ```date``` and ```float``` types. Even though you didn't change this part, but it should be not allowed.
* The addition and subtraction of ```time``` and ```timestamp``` should be allowed. Please see the below.
```
default> \d test
table name: default.test
table uri: hdfs://localhost:7020/tajo/warehouse/default/test
store type: TEXT
number of rows: 1
volume: 29 B
Options:
'timezone'='Asia/Seoul'
'text.delimiter'='|'
schema:
t TIME
ts TIMESTAMP
default> select t + ts from test;
ERROR: operator does not exist: 'default.test.t (TIME) + default.test.ts (TIMESTAMP)'
```
* The below error message looks not appropriate.
```
default> select '1990-01-01'::date + '1990-01-01 00:10:10'::timestamp;
ERROR: internal error: Cannot compare to TIMESTAMP type datum
```
* The following cases should not be allowed.
```
default> select '00:10:10'::timestamp;
?cast
-------------------------------
0002-11-30 00:10:10 BC
default> select '00:10:10'::date;
?cast
-------------------------------
0002-11-30
(1 rows, 0.003 sec, 0 B selected)
default> select '1992-01-01'::time;
?cast
-------------------------------
1992:01:01
(1 rows, 0.003 sec, 0 B selected)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60341994
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/datetime/DatePartFromTimestamp.java ---
@@ -59,8 +55,9 @@ public DatePartFromTimestamp() {
@Override
public void init(OverridableConf context, FunctionEval.ParamType [] types) {
- String timezoneId = context.get(SessionVars.TIMEZONE, TajoConstants.DEFAULT_SYSTEM_TIMEZONE);
- timezone = TimeZone.getTimeZone(timezoneId);
+ if (!hasTimeZone()) {
--- End diff --
Actually, this behavior looks to be intended. However, it also looks weird. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60546521
--- Diff: tajo-core/src/main/java/org/apache/tajo/master/exec/QueryExecutor.java ---
@@ -327,6 +328,11 @@ public void execNonFromQuery(QueryContext queryContext, Session session, String
LogicalRootNode rootNode = plan.getRootBlock().getRoot();
EvalContext evalContext = new EvalContext();
+
+ //Non From query should be session's time zone. e,g, select to_char(now(), 'yyyy-MM-dd')
+ String timezoneId = queryContext.get(SessionVars.TIMEZONE);
--- End diff --
Right. Thanks for your answer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60180863
--- Diff: tajo-client/src/main/java/org/apache/tajo/jdbc/TajoResultSetBase.java ---
@@ -364,10 +364,16 @@ private Timestamp getTimestamp(Tuple tuple, TimeZone tz, int index) throws SQLEx
}
private Timestamp toTimestamp(TimeMeta tm, TimeZone tz) {
+
+ long javaTime = DateTimeUtil.julianTimeToJavaTime(DateTimeUtil.toJulianTimestamp(tm));
--- End diff --
It looks inconvenient to call both functions whenever we want to convert TimeMeta into java time. How about adding a function which simply wraps these calls?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60343969
--- Diff: tajo-storage/tajo-storage-common/src/main/java/org/apache/tajo/storage/TextSerializerDeserializer.java ---
@@ -101,12 +102,12 @@ public int serialize(int index, Tuple tuple, OutputStream out, byte[] nullCharac
out.write(bytes);
break;
case TIME:
- bytes = TimeDatum.asChars(tuple.getTimeDate(index), TimeZone.getDefault(), true).getBytes();
+ bytes = tuple.getTextBytes(index);
length = bytes.length;
out.write(bytes);
break;
case TIMESTAMP:
- bytes = TimestampDatum.asChars(tuple.getTimeDate(index), TimeZone.getDefault(), true).getBytes();
+ bytes = TimestampDatum.asChars(tuple.getTimeDate(index), TAJO_CONF.getSystemTimezone(), true).getBytes();
--- End diff --
Looks to be TableMeta's timezone.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60346629
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/datetime/DatePartFromTimestamp.java ---
@@ -59,8 +55,9 @@ public DatePartFromTimestamp() {
@Override
public void init(OverridableConf context, FunctionEval.ParamType [] types) {
- String timezoneId = context.get(SessionVars.TIMEZONE, TajoConstants.DEFAULT_SYSTEM_TIMEZONE);
- timezone = TimeZone.getTimeZone(timezoneId);
+ if (!hasTimeZone()) {
--- End diff --
I think date_part, add_month and add_day should be client timezone when date type is cast to timestamp
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212225990
@jinossy, thanks for your quick response. I agree on the need of parser validator.
In addition to the above comments, I found another case in which the result is not the one I expected.
Here is an example.
```
default> create table test2 (ts timestamp, d date, t time) with ('timezone'='GMT-5');
OK
default> \d test2
table name: default.test2
table uri: hdfs://localhost:7020/tajo/warehouse/default/test2
store type: TEXT
number of rows: 0
volume: 0 B
Options:
'timezone'='GMT-5'
'text.delimiter'='|'
schema:
ts TIMESTAMP
d DATE
t TIME
default> insert into test2 select '2016-05-01 02:02:33'::timestamp, '2016-05-01':: date, '02:02:33'::time;
OK
default> \set
Invalid command \se. Try \? for help.
'SESSION_LAST_ACCESS_TIME'='1461120772913'
'CURRENT_DATABASE'='default'
'USERNAME'='jihoon'
'SESSION_ID'='351f5cb9-7dea-4277-9041-46962439622e'
'TIMEZONE'='Asia/Seoul'
'FETCH_ROWNUM'='200'
'COMPRESSED_RESULT_TRANSFER'='false'
default> select * from test2;
ts, d, t
-------------------------------
2016-05-01 02:02:33, 2016-05-01, 02:02:33
(1 rows, 0.028 sec, 0 B selected)
default> \set TIMEZONE GMT-5
default> select * from test2;
ts, d, t
-------------------------------
2016-04-30 12:02:33, 2016-05-01, 02:02:33
(1 rows, 0.014 sec, 0 B selected)
```
Even though I created the table ```test2``` with the timezone of 'GMT-5', the result value is the inserted one only when the client timezone is 'GMT+9'.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212240588
You are right. In that example, I inserted constant data which is affected by the client timezone.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212265382
You should change to interval '5 day'
It looks like alias bug
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-211974078
Thanks for your detailed review!
I’ve update the patch that reflects your comments but IMO, we should add a validator of datetime parser to the other ticket
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60519338
--- Diff: tajo-core/src/main/java/org/apache/tajo/master/exec/QueryExecutor.java ---
@@ -327,6 +328,11 @@ public void execNonFromQuery(QueryContext queryContext, Session session, String
LogicalRootNode rootNode = plan.getRootBlock().getRoot();
EvalContext evalContext = new EvalContext();
+
+ //Non From query should be session's time zone. e,g, select to_char(now(), 'yyyy-MM-dd')
+ String timezoneId = queryContext.get(SessionVars.TIMEZONE);
--- End diff --
Non form query should be used SessionVars.TIMEZONE and some builtin functions(now, date_part ..) should be same
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60341468
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/datetime/AddDays.java ---
@@ -52,8 +56,16 @@ public AddDays() {
}
@Override
+ public void init(OverridableConf context, FunctionEval.ParamType[] types) {
+ if (!hasTimeZone()) {
+ setTimeZone(context.getConf().getSystemTimezone());
--- End diff --
My test result was correct. Could you test again?
```
default> \d test;
table name: default.test
table uri: hdfs://localhost:9010/tajo/warehouse/default/test
store type: TEXT
number of rows: 1
volume: 40 B
Options:
'timezone'='Asia/Seoul'
'text.delimiter'='|'
schema:
t TIME
ts TIMESTAMP
d DATE
default> select * from test;
t, ts, d
-------------------------------
01:01:23, 2015-05-01 01:01:23, 2015-05-01
02:02:23, 2016-05-01 02:02:23, 2016-05-01
(2 rows, 0.154 sec, 0 B selected)
default> select add_days(ts, 5) from test;
?add_days
-------------------------------
2015-05-06 01:01:23
2016-05-06 02:02:23
(2 rows, 0.076 sec, 0 B selected)
default> select add_days(d, 5) from test;
?add_days
-------------------------------
2015-05-06 00:00:00
2016-05-06 00:00:00
(2 rows, 0.03 sec, 0 B selected)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-208189663
I’ve change time type to time type without timezone because it can't +/- operation.
Actually we should add timetz type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212274357
Never mind! I've update the patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60518990
--- Diff: tajo-common/src/main/java/org/apache/tajo/datum/DatumFactory.java ---
@@ -343,9 +342,6 @@ public static TimeDatum createTime(Datum datum, @Nullable TimeZone tz) {
case VARCHAR:
case TEXT:
TimeMeta tm = DateTimeFormat.parseDateTime(datum.asChars(), "HH24:MI:SS.MS");
- if (tz != null) {
--- End diff --
done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60182642
--- Diff: tajo-client/src/main/java/org/apache/tajo/jdbc/TajoResultSetBase.java ---
@@ -364,10 +364,16 @@ private Timestamp getTimestamp(Tuple tuple, TimeZone tz, int index) throws SQLEx
}
private Timestamp toTimestamp(TimeMeta tm, TimeZone tz) {
+
+ long javaTime = DateTimeUtil.julianTimeToJavaTime(DateTimeUtil.toJulianTimestamp(tm));
--- End diff --
Good idea! I will add a function that hiding these code
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/993
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60182577
--- Diff: tajo-common/src/main/java/org/apache/tajo/datum/DatumFactory.java ---
@@ -285,7 +285,6 @@ public static TimeDatum createTime(String timeStr) {
public static TimeDatum createTime(String timeStr, TimeZone tz) {
--- End diff --
This method is not used anymore. Would you remove it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60339843
--- Diff: tajo-cluster-tests/src/test/java/org/apache/tajo/QueryTestCaseBase.java ---
@@ -265,6 +262,11 @@ public void printTestName() {
name.getMethodName()));
}
+ @After
+ public void clear() {
+ getClient().unsetSessionVariables(Lists.newArrayList("TIMEZONE"));
--- End diff --
It will be better to use SessionVars.TIMEZONE.name().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212263282
Here is another bug. The result of addition of ```timestamp``` or ```date``` with ```interval``` is invalid.
```
default> \set
...
'TIMEZONE'='Asia/Seoul'
...
default> \d test
table name: default.test
table uri: hdfs://localhost:7020/tajo/warehouse/default/test
store type: TEXT
number of rows: 1
volume: 52 B
Options:
'timezone'='Asia/Seoul'
'text.delimiter'='|'
schema:
t TIME
ts TIMESTAMP
d DATE
default> select ts from test;
ts
-------------------------------
2015-05-01 15:01:23
2016-05-01 16:02:33
(2 rows, 0.03 sec, 0 B selected)
default> select ts + interval '5' day from test;
day
-------------------------------
2015-05-01 15:01:23
2016-05-01 16:02:33
(2 rows, 0.02 sec, 0 B selected)
default> select d from test;
d
-------------------------------
2015-05-01
2016-05-01
(2 rows, 0.018 sec, 0 B selected)
default> select d + interval '5' day from test;
day
-------------------------------
2015-05-01 00:00:00
2016-05-01 00:00:00
(2 rows, 0.016 sec, 0 B selected)
default> select ts from test;
ts
-------------------------------
2015-05-01 15:01:23
2016-05-01 16:02:33
(2 rows, 0.018 sec, 0 B selected)
default> select ts - interval '5' day from test;
day
-------------------------------
2015-05-01 15:01:23
2016-05-01 16:02:33
(2 rows, 0.014 sec, 0 B selected)
default> select d from test;
d
-------------------------------
2015-05-01
2016-05-01
(2 rows, 0.027 sec, 0 B selected)
default> select d - interval '5' day from test;
day
-------------------------------
2015-05-01 00:00:00
2016-05-01 00:00:00
(2 rows, 0.017 sec, 0 B selected)
```
It is weird because tests for interval datum are already included.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60341436
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/datetime/DatePartFromTimestamp.java ---
@@ -59,8 +55,9 @@ public DatePartFromTimestamp() {
@Override
public void init(OverridableConf context, FunctionEval.ParamType [] types) {
- String timezoneId = context.get(SessionVars.TIMEZONE, TajoConstants.DEFAULT_SYSTEM_TIMEZONE);
- timezone = TimeZone.getTimeZone(timezoneId);
+ if (!hasTimeZone()) {
--- End diff --
This function has some bugs.
```
default> \d test
table name: default.test
table uri: hdfs://localhost:7020/tajo/warehouse/default/test
store type: TEXT
number of rows: 1
volume: 40 B
Options:
'timezone'='Asia/Seoul'
'text.delimiter'='|'
schema:
t TIME
ts TIMESTAMP
d DATE
default> \set TIMEZONE GMT-9
default> select ts from test;
ts
-------------------------------
2015-04-30 21:01:23
2016-04-30 22:02:33
(2 rows, 0.016 sec, 0 B selected)
default> select date_part('day', ts) from test;
?date_part
-------------------------------
1.0
1.0
(2 rows, 0.021 sec, 0 B selected)
default> select date_part('hour', ts) from test;
?date_part
-------------------------------
15.0
16.0
(2 rows, 0.024 sec, 0 B selected)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60346243
--- Diff: tajo-storage/tajo-storage-common/src/main/java/org/apache/tajo/storage/TextSerializerDeserializer.java ---
@@ -101,12 +102,12 @@ public int serialize(int index, Tuple tuple, OutputStream out, byte[] nullCharac
out.write(bytes);
break;
case TIME:
- bytes = TimeDatum.asChars(tuple.getTimeDate(index), TimeZone.getDefault(), true).getBytes();
+ bytes = tuple.getTextBytes(index);
length = bytes.length;
out.write(bytes);
break;
case TIMESTAMP:
- bytes = TimestampDatum.asChars(tuple.getTimeDate(index), TimeZone.getDefault(), true).getBytes();
+ bytes = TimestampDatum.asChars(tuple.getTimeDate(index), TAJO_CONF.getSystemTimezone(), true).getBytes();
--- End diff --
TextSerializerDeserializer does not support table meta and it was deprecated
IMO, we should remove the timestamp type in TextSerializerDeserializer
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212266207
Oh, right.. I'm little bit ashamed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60346938
--- Diff: tajo-storage/tajo-storage-common/src/main/java/org/apache/tajo/storage/TextSerializerDeserializer.java ---
@@ -101,12 +102,12 @@ public int serialize(int index, Tuple tuple, OutputStream out, byte[] nullCharac
out.write(bytes);
break;
case TIME:
- bytes = TimeDatum.asChars(tuple.getTimeDate(index), TimeZone.getDefault(), true).getBytes();
+ bytes = tuple.getTextBytes(index);
length = bytes.length;
out.write(bytes);
break;
case TIMESTAMP:
- bytes = TimestampDatum.asChars(tuple.getTimeDate(index), TimeZone.getDefault(), true).getBytes();
+ bytes = TimestampDatum.asChars(tuple.getTimeDate(index), TAJO_CONF.getSystemTimezone(), true).getBytes();
--- End diff --
Right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60182705
--- Diff: tajo-common/src/main/java/org/apache/tajo/datum/DatumFactory.java ---
@@ -343,9 +342,6 @@ public static TimeDatum createTime(Datum datum, @Nullable TimeZone tz) {
case VARCHAR:
case TEXT:
TimeMeta tm = DateTimeFormat.parseDateTime(datum.asChars(), "HH24:MI:SS.MS");
- if (tz != null) {
--- End diff --
```tz``` parameter is not used anymore. Would you remove it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212714373
I created new ticket about validator https://issues.apache.org/jira/browse/TAJO-2128
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-215008937
+1, thanks for your effort!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-212237190
@jihoonson
I've convert your testing example to pgsql
please refer to following sequence:
```
postgres=# set time zone -5;
postgres=# insert into test select time '02:02:23', timestamptz '2016-05-01 02:02:23+9', date '2016-05-01’;
postgres=# set time zone +9; //restore your default timezone
postgres=# select * from test;
t | ts | d
----------+------------------------+------------
02:02:23 | 2016-05-01 02:02:23+09 | 2016-05-01
postgres=# set time zone -5;
postgres=# select * from test;
t | ts | d
----------+------------------------+------------
02:02:23 | 2016-04-30 12:02:23-05 | 2016-05-01
(1 rows)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/993#issuecomment-211823217
And I'm still reviewing. I'll finish soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60358157
--- Diff: tajo-core/src/main/java/org/apache/tajo/master/exec/QueryExecutor.java ---
@@ -327,6 +328,11 @@ public void execNonFromQuery(QueryContext queryContext, Session session, String
LogicalRootNode rootNode = plan.getRootBlock().getRoot();
EvalContext evalContext = new EvalContext();
+
+ //Non From query should be session's time zone. e,g, select to_char(now(), 'yyyy-MM-dd')
+ String timezoneId = queryContext.get(SessionVars.TIMEZONE);
--- End diff --
SessionVars.TIMEZONE is designed to be used only in client and master, and QueryVars are disigned to use some variables during query execution. How about adding a new configuration key for timezone to QueryVars?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2110: Fix incorrect DateTime and remove ha...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/993#discussion_r60351294
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/function/datetime/DatePartFromTimestamp.java ---
@@ -59,8 +55,9 @@ public DatePartFromTimestamp() {
@Override
public void init(OverridableConf context, FunctionEval.ParamType [] types) {
- String timezoneId = context.get(SessionVars.TIMEZONE, TajoConstants.DEFAULT_SYSTEM_TIMEZONE);
- timezone = TimeZone.getTimeZone(timezoneId);
+ if (!hasTimeZone()) {
--- End diff --
It looks a bug to me. As you know, the reason why the results of the above queries are different is there is no way to apply the client timezone to the result of date_part(). This is also same for add_month() and add_day() functions. I think we need to fix this too because the result is not the one which users usually expect. Of course, the result is different from that of pgsql.
But, this also looks to require a huge amount of work. If you agree, how about handling this issue in another Jira?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---