[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user samarthjain commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-87109980 Tests ran fine for me locally so I went ahead and committed 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user samarthjain commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-87022884 @gabrielreid - does this look good for commit now? We are planning on cutting the RC for 4.3.1 pretty 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user tzolkincz commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-86985696 yes my bad. Fixed --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user tzolkincz commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-86913995 I'm on it, it looks strange. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user gabrielreid commented on the pull request:
https://github.com/apache/phoenix/pull/42#issuecomment-86879632
@tzolkincz I just tried running this on a Phoenix install, and there seems
to be an issue with the code now if I use the CONVERT_TZ function over multiple
rows.
I'm able to replicate it in ConvertTimezoneFunctionIT with the following
test:
@Test
public void testConvertMultipleRecords() throws Exception {
Connection conn = DriverManager.getConnection(getUrl());
String ddl = "CREATE TABLE IF NOT EXISTS TIMEZONE_OFFSET_TEST (k1
INTEGER NOT NULL, dates DATE CONSTRAINT pk PRIMARY KEY (k1))";
Statement stmt = conn.createStatement();
stmt.execute(ddl);
stmt.execute( "UPSERT INTO TIMEZONE_OFFSET_TEST (k1, dates) VALUES
(1, TO_DATE('2014-03-01 00:00:00'))");
stmt.execute( "UPSERT INTO TIMEZONE_OFFSET_TEST (k1, dates) VALUES
(2, TO_DATE('2014-03-01 00:00:00'))");
conn.commit();
ResultSet rs = stmt.executeQuery(
"SELECT k1, dates, CONVERT_TZ(dates, 'UTC', 'America/Adak')
FROM TIMEZONE_OFFSET_TEST");
assertTrue(rs.next());
assertEquals(139359600L, rs.getDate(3).getTime()); //Fri, 28
Feb 2014 14:00:00
assertTrue(rs.next());
assertEquals(139359600L, rs.getDate(3).getTime()); //Fri, 28
Feb 2014 14:00:00
assertFalse(rs.next());
}
Running that test results in the following:
java.sql.SQLException: ERROR 201 (22000): Illegal data. Unknown
timezone � Dx�g Adak
at
org.apache.phoenix.exception.SQLExceptionCode$Factory$1.newException(SQLExceptionCode.java:362)
at
org.apache.phoenix.exception.SQLExceptionInfo.buildException(SQLExceptionInfo.java:133)
at
org.apache.phoenix.schema.IllegalDataException.(IllegalDataException.java:38)
at
org.apache.phoenix.cache.JodaTimezoneCache.getInstance(JodaTimezoneCache.java:48)
at
org.apache.phoenix.cache.JodaTimezoneCache.getInstance(JodaTimezoneCache.java:60)
at
org.apache.phoenix.expression.function.ConvertTimezoneFunction.evaluate(ConvertTimezoneFunction.java:70)
at
org.apache.phoenix.compile.ExpressionProjector.getValue(ExpressionProjector.java:69)
at
org.apache.phoenix.jdbc.PhoenixResultSet.getDate(PhoenixResultSet.java:356)
at
org.apache.phoenix.end2end.ConvertTimezoneFunctionIT.testConvertMultipleRecords(ConvertTimezoneFunctionIT.java:80)
Could you look into what's going wrong?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user JamesRTaylor commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-86829091 @gabrielreid - if this looks good to you, please commit to 4.3 and the other branches. Thanks for the contributions and reviews! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user tzolkincz commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-86740884 Thanks for notification. I'm done with my changes. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user JamesRTaylor commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-86683996 @tzolkincz - we're about to cut a 4.3.1 RC, so if you need this in, we'll need it today. Otherwise, 4.4.0 may be your next release vehicle. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user gabrielreid commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-85937432 Making it more abstract and not having duplication of code definitely sounds like the way to go, if you're up for doing that it would be great. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user tzolkincz commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-85911379 Yes, it should be thread safe but we could consider to use same lib for timezones and same caching mechanism. So if you are for duplication of [caching](https://github.com/apache/phoenix/pull/42/files#diff-83862191fb126a769d5fd74be3534d90R98) code, I'll do that. It is small fraction of code, but I'd rather make it more abstract. However it's not major question here. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user gabrielreid commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-85894438 I don't think it's the same issue in TIMEZONE_OFFSET because the timezone cache isn't static, so (as far as I know) it will only be accessed by a single thread. That being said, I think it makes sense to make the same change there as well, if you're up for doing that. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user tzolkincz commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-85680197 Thank you for review @gabrielreid, i've solved that. I realized [TIMEZONE_OFFSET](https://github.com/apache/phoenix/blob/master/phoenix-core/src/main/java/org/apache/phoenix/expression/function/TimezoneOffsetFunction.java#L48) have similar issue. I think we should rather extract timezone cache mechanism and make it as part of org.apache.phoenix.cache. Or are there some other use-cases of (instance) cache so we could implement it more generally? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user gabrielreid commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-85670735 Patch looks good, but I've got the one remark/question that I put on the commit. Can you take a look at that and let me know what 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
Github user tzolkincz commented on the pull request: https://github.com/apache/phoenix/pull/42#issuecomment-84464732 @gabrielreid I made changes as you suggested. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
GitHub user tzolkincz opened a pull request: https://github.com/apache/phoenix/pull/43 PHOENIX-1722 Speedup CONVERT_TZ function Using Joda Time lib instead of java.util.TimeZone. This would speedup this function more than 3 times. I've also updated version of Joda Time to 2.7 cause of some timezone bugfixes and speedups. You can merge this pull request into a Git repository by running: $ git pull https://github.com/tzolkincz/phoenix convert_tz_speedup_4.3 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/43.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 #43 commit 275b34706960c3052374a671bfe2a496470df648 Author: Vaclav Loffelmann Date: 2015-03-09T14:50:22Z PHOENIX-1722 Speedup CONVERT_TZ function --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-1722 Speedup CONVERT_TZ function
GitHub user tzolkincz opened a pull request: https://github.com/apache/phoenix/pull/42 PHOENIX-1722 Speedup CONVERT_TZ function Using Joda Time lib instead of java.util.TimeZone. This would speedup this function more than 3 times. I've also updated version of Joda Time to 2.7 cause of some timezone bugfixes and speedups. You can merge this pull request into a Git repository by running: $ git pull https://github.com/tzolkincz/phoenix convert_tz_speedup Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/42.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 #42 commit 8d8ec2e727af327196a2af8803c6676f48c84a63 Author: Vaclav Loffelmann Date: 2015-03-09T14:50:22Z PHOENIX-1722 Speedup CONVERT_TZ function --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
