You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by tzolkincz <gi...@git.apache.org> on 2014/04/02 18:52:07 UTC
[GitHub] incubator-phoenix pull request: add function timezone offset
GitHub user tzolkincz opened a pull request:
https://github.com/apache/incubator-phoenix/pull/26
add function timezone offset
Function for get offset (shift) of timezone at particular datetime. Example use case: let's say you have stored some data in some offset and want to query like this:
```SQL
SELECT * FROM table
WHERE offset = TIMEZONE_OFFSET('Indian/Cocos', TO_DATE('2014-02-01 00:00:00'))
```
Thanks
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tzolkincz/incubator-phoenix timezoneOffset
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-phoenix/pull/26.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 #26
----
commit fa8b0f8483e4f4994b0b13dff626cb52d9acc4d0
Author: tzolkincz <va...@socialbakers.com>
Date: 2014-04-02T16:19:35Z
add function timezone offset
----
---
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] incubator-phoenix pull request: add function timezone offset
Posted by tzolkincz <gi...@git.apache.org>.
Github user tzolkincz commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-42187381
Finally I see your point. So, there is a situation:
* if we use Timestamp, we'll have nanoseconds time resolution which implicate plus 4 bytes of data when NONE BLOCK_DATA_ENCODING and aprox 2 bytes more when use FAST_DIFF DATA_ENCODING. It is too much data, when it is in row key.
* If we use Date, it' wont be compatible with JDBC, so there is another question: What will current Phoenix Date implementation become?
We would prefer use some date time format compatible with JDBC with reasonable size (8 bytes). What is your suggestion to store lot of date time data where format yyyy-mm-dd hh:mm:ss is fair enough?
---
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] incubator-phoenix pull request: add function timezone offset
Posted by tzolkincz <gi...@git.apache.org>.
Github user tzolkincz commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-42016094
HI, thanks for code review.
* I've updated code to parsing Date object not Long object. So if implementation of Phoenix Date will change, this will still works. It makes sense to use date cos you wanna get offset by timezone in some particular date - so you'll probably call this function on column with Date data type.
* This function returns minutes because minutes is lowest resolution of shifting one timezone to another. For example http://docs.oracle.com/cd/B19306_01/server.102/b14200/functions202.htm Oracle function returns varchar in format HH:MM, but we are doing some calculations with result of this function, so some number type is better.
* I've added implementation of isNullable method and some test for check null as input arguments. It works as other functions, it returns null.
Our use case is following: We store data per date and per offset. When querying, we want data per timezone (one timezone can have multiple offsets per year (daylight save time)) so our query looks like ```... WHERE TIMEZONE_OFFSET('Asia/Almaty', date) = offset ... ```
---
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] incubator-phoenix pull request: add function timezone offset
Posted by gabrielreid <gi...@git.apache.org>.
Github user gabrielreid commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-41725018
A couple of points on this:
* I think that this should probably take a Timestamp as parameter instead of a Date. Although Dates and Timestamps are currently internally equivalent in Phoenix, this isn't the case in JDBC itself, and may change in Phoenix in the future as well.
* I'm wondering what the rationale is for having the return value in minutes -- I'm not totally clear on the end use case of this function, but seeing as timestamps are typically dealt with in terms of milliseconds, it seems as though it would likely be more useful to have a return value in milliseconds
* It looks like there will be issues if either of the parameters is null. The null handling for both/either of the parameters should probably be demonstrated in the tests.
Could you explain a bit more about how you see this function being used? I'm wondering if there is a more streamlined way of defining a function to achieve the same end goal.
---
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] incubator-phoenix pull request: add function timezone offset
Posted by gabrielreid <gi...@git.apache.org>.
Github user gabrielreid commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-42072847
> It makes sense to use date cos you wanna get offset by timezone in some particular date - so you'll probably call this function on column with Date data type
Timezones change for daylight savings at various times of day (typically at night), so a single date can have multiple timezone offsets. For this reason, it makes more sense to me to use a Timestamp as the parameter 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-phoenix pull request: add function timezone offset
Posted by gabrielreid <gi...@git.apache.org>.
Github user gabrielreid commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-42192917
I think that Timestamp is the only applicable JDBC type that is appropriate for this kind of function -- it's the only thing that represents a specific point in time, which is what's needed here.
The fact that Timestamp is larger than desired for a value in the row key is an application-specific issue that (probably) shouldn't play too much of a role in defining the API for the function. However, I think there are ways around this -- for example, assuming that the intention is to get the time offset at midnight of the given date, I think you could just cast the date values to timestamps in your SQL statements.
---
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] incubator-phoenix pull request: add function timezone offset
Posted by gabrielreid <gi...@git.apache.org>.
Github user gabrielreid commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-42276153
Sounds ok to me.
At first I was thinking that this is something that should really be avoided (i.e. making what is essentially an API method with something that won't be backwards compatible), but it is indeed Phoenix specific, and if the Date/Time/Timestamp handling in Phoenix is fully brought in line with JDBC then there will be quite a few other migration topics as well, and this can be taken care of then.
---
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] incubator-phoenix pull request: add function timezone offset
Posted by tzolkincz <gi...@git.apache.org>.
Github user tzolkincz closed the pull request at:
https://github.com/apache/incubator-phoenix/pull/26
---
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] incubator-phoenix pull request: add function timezone offset
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-42208259
+1 @tzolkincz. I think this approach makes sense.
---
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] incubator-phoenix pull request: add function timezone offset
Posted by jtaylor-sfdc <gi...@git.apache.org>.
Github user jtaylor-sfdc commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-43171342
This looks good to me. @gabrielreid - if you agree, would you mind pulling it in?
---
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] incubator-phoenix pull request: add function timezone offset
Posted by tzolkincz <gi...@git.apache.org>.
Github user tzolkincz commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-42206344
For now I would keep implementation with Date, as long as Phoenix Date is suitable for this use case. When we switch Date in Phoenix to JDBC date, we would migrate this (and other functionality) to new/most suitable data type (i mean like datetime in other databases).
I don't actually see any advantage with casting in query, it'll only do query more complicated than is required. I think we should rather consider to create DATETIME data 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] incubator-phoenix pull request: add function timezone offset
Posted by gabrielreid <gi...@git.apache.org>.
Github user gabrielreid commented on the pull request:
https://github.com/apache/incubator-phoenix/pull/26#issuecomment-43331422
This has been logged in Jira (https://issues.apache.org/jira/browse/PHOENIX-983) and pulled in.
I don't have the rights to close this PR, could you do that @jtaylor-sfdc or @tzolkincz ?
---
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.
---