feat: New UNIX_TIMESTAMP and UNIX_DATE datetime functions #2459

mmolimar · 2019-02-20T06:09:13Z

Description

This PR adds two new datetime functions: UNIX_TIMESTAMP and UNIX_DATE.
The issue related with this is #2412 by @rmoff.

Testing done

Added new unit tests

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

ghost · 2019-02-20T06:09:15Z

It looks like @mmolimar hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

mmolimar · 2019-02-20T06:21:05Z

[clabot:check]

ghost · 2019-02-20T06:21:06Z

@confluentinc It looks like @mmolimar just signed our Contributor License Agreement. 👍

Always at your service,

clabot

spena · 2019-02-20T15:26:43Z

Thanks @mmolimar for the patch. I was looking at other SQL systems, and all of them use a CURRENT_DATE and CURRENT_TIMESTAMP functions and return different results other than a number. Should we follow the same standard?

Btw, I think it is useful to return a DATE and TIMESTAMP data types (like other SQL do) to allow for date/time calculations but KSQL does not support those types yet. Would it be useful to have those first?

If you want to return a timestamp number instead for current date/time support, perhaps having a different UDF name would be ideal? So we don't have compatibility troubles in the future if we want to use these UDF to return date/time types.

For instance, Apache Hive has a unix_timestamp() that returns an integer.

docs/developer-guide/syntax-reference.rst

JimGalasyn

Looks good, just a few doc suggestions.

mmolimar · 2019-02-21T01:04:08Z

Totally agree @spena.
In case of CURRENTTIMESTAMP, I could rename it to UNIXTIMESTAMP (btw, with underscores? Following the pattern in other functions, most of them don't have it), but what about the CURRENTDATE?

On the other hand, we have other functions such as STRINGTOTIMESTAMP or STRINGTODATE in which the thing related with the data type happens too. That's why I implemented those functions in the same way so, what do you say?

hjafarpour

Thanks for the PR, @mmolimar.
I agree with @spena 's suggestion on the name. Also since currently we don't have DATE and TIME types in KSQL, so these UDFs don't have to follow the semantics of the other SQL systems.
One important point to add to the documentation is to clarify that the returned time is from the local KSQL server and if we a cluster with more than one server the local time for the servers may not be synchronized.

ksql-engine/src/main/java/io/confluent/ksql/function/udf/datetime/CurrentTimestamp.java

ksql-engine/src/test/java/io/confluent/ksql/function/udf/datetime/CurrentTimestampTest.java

spena · 2019-03-08T15:41:37Z

@mmolimar @hjafarpour I did not find a current_date() function in any SQL database, however, I wonder what would be the difference between current_timestamp (or unix_timestamp) and current_date? I think a user can get the # of days based on the value of unix_timestamp(), right? They can get the difference between unix_timestamp() - epoc and calculate the days if necessary.

Btw, I came to this problem:
SELECT c1 + unix_timestamp(), c2 + unix_timestamp() FROM t1;

Would both unix_timestamp() return different timestamp values? If the idea is to add the current timestamp to those columns, then one column might be a few milliseconds greater than the other. Some systems do return the same timestamp value in the same query to avoid such problem.

It's hard to test this in Mysql because it is too fast and I always get the same values, but in KSQL there might be operations in the query that would make the next timestamp to be different.

…ature-datetimefn

mmolimar · 2019-03-15T05:26:51Z

@hjafarpour @spena I renamed the function CURRENTTIMESTAMP to UNIX_TIMESTAMP as suggested.
On the other hand, I did find the current_date function, for instance, in MySQL and PostgreSQL so, what do you think?

HLmohammed · 2019-04-25T02:01:07Z

@JimGalasyn , @mmolimar , waiting on these functions to be merged bigtime. Hope it happens soon.

JimGalasyn · 2019-04-25T14:59:54Z

docs/developer-guide/syntax-reference.rst

+|                        |                                                                           | ``java.util.TimeZone`` ID format, for example:    |
+|                        |                                                                           | "UTC", "America/Los_Angeles", "PDT",              |
+|                        |                                                                           | "Europe/London".                                  |
+|                        |                                                                           | Notice that the returned timestamp might be       |


Suggest: "The returned timestamp may differ depending on the local time of different KSQL Server instances."

JimGalasyn

LGTM, with one suggestion.

…ature-datetimefn

hjafarpour

LGTM with one suggestion.

hjafarpour · 2019-06-18T22:49:53Z

ksql-engine/src/test/java/io/confluent/ksql/function/udf/datetime/UnixTimestampTest.java

+    // Given:
+    final long before = System.currentTimeMillis();
+
+    // When:


It would be good if you refactor the code so that you could mock the timestamp generation and make the tests more deterministic.

Normally I'd agree, but in this case if we change the test to inject the time we're not really testing the class anymore, given its sole purpose is to provide the time!

(Note, this is in response to @hjafarpour's comment above)

big-andy-coates

Thanks @mmolimar, really appreciate you taking the time to add this in.

However, as I've commented below, I don't think it makes sense to have a timezone parameter on the UNIX_TIMESTAMP function, as the timestamp is independent of any tz.

If we can clear this up I think we can get this merged.

Thanks

docs/developer-guide/syntax-reference.rst

ksql-engine/src/main/java/io/confluent/ksql/function/udf/datetime/UnixTimestamp.java

big-andy-coates · 2019-06-19T10:59:33Z

ksql-engine/src/test/java/io/confluent/ksql/function/udf/datetime/UnixTimestampTest.java

+    // Given:
+    final long before = System.currentTimeMillis();
+
+    // When:


Normally I'd agree, but in this case if we change the test to inject the time we're not really testing the class anymore, given its sole purpose is to provide the time!

(Note, this is in response to @hjafarpour's comment above)

…ature-datetimefn

mmolimar · 2019-06-26T22:24:10Z

Totally agree @big-andy-coates
Changes done!

big-andy-coates · 2019-06-27T07:44:18Z

Brilliant - thanks @mmolimar. Really appreciate your contribution. Will merge once build is green.

big-andy-coates

LGTM

mmolimar added 2 commits February 19, 2019 23:57

Added new currenttimestamp and currentdate functions

bf3191c

Updating syntax reference

671bb3c

mmolimar requested review from JimGalasyn and a team as code owners February 20, 2019 06:09

JimGalasyn reviewed Feb 20, 2019

View reviewed changes

docs/developer-guide/syntax-reference.rst Outdated Show resolved Hide resolved

JimGalasyn reviewed Feb 20, 2019

View reviewed changes

docs/developer-guide/syntax-reference.rst Outdated Show resolved Hide resolved

JimGalasyn reviewed Feb 20, 2019

View reviewed changes

docs/developer-guide/syntax-reference.rst Outdated Show resolved Hide resolved

JimGalasyn requested changes Feb 20, 2019

View reviewed changes

Changing descriptions in docs

131802b

hjafarpour reviewed Mar 6, 2019

View reviewed changes

ksql-engine/src/main/java/io/confluent/ksql/function/udf/datetime/CurrentTimestamp.java Outdated Show resolved Hide resolved

ksql-engine/src/test/java/io/confluent/ksql/function/udf/datetime/CurrentTimestampTest.java Outdated Show resolved Hide resolved

mmolimar added 2 commits March 14, 2019 23:21

Merge branch 'master' of https://github.com/confluentinc/ksql into fe…

91a83ee

…ature-datetimefn

Renamed currenttimestamp function to unix_timestamp

b13d9df

JimGalasyn reviewed Apr 25, 2019

View reviewed changes

JimGalasyn approved these changes Apr 25, 2019

View reviewed changes

mmolimar added 2 commits April 25, 2019 17:07

Merge branch 'master' of https://github.com/confluentinc/ksql into fe…

c9005d6

…ature-datetimefn

Updating documentation

51b53a9

vcrfxia requested review from spena, hjafarpour and a team and removed request for hjafarpour May 7, 2019 20:51

Merge branch 'master' of https://github.com/confluentinc/ksql into fe…

2afc64f

…ature-datetimefn

hjafarpour approved these changes Jun 18, 2019

View reviewed changes

big-andy-coates reviewed Jun 19, 2019

View reviewed changes

mmolimar added 2 commits June 25, 2019 23:50

Merge branch 'master' of https://github.com/confluentinc/ksql into fe…

85c56df

…ature-datetimefn

Renaming currentdate function to unix_date

6a1ff15

big-andy-coates added 2 commits June 27, 2019 08:42

Update docs to show return value of UNIX_DATE can vary

60ddd3b

Remove duplicate cate.

ad31836

big-andy-coates approved these changes Jun 27, 2019

View reviewed changes

vcrfxia changed the title ~~New CURRENTIMESTAMP and CURRENTDATE datetime functions~~ feat: New CURRENTIMESTAMP and CURRENTDATE datetime functions Jun 27, 2019

vcrfxia changed the title ~~feat: New CURRENTIMESTAMP and CURRENTDATE datetime functions~~ feat: New UNIX_TIMESTAMP and UNIX_DATE datetime functions Jun 27, 2019

docs: update changelog

9c79f10

vcrfxia merged commit 39ce7f4 into confluentinc:master Jun 27, 2019

vcrfxia added this to the 5.4 milestone Jul 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New UNIX_TIMESTAMP and UNIX_DATE datetime functions #2459

feat: New UNIX_TIMESTAMP and UNIX_DATE datetime functions #2459

mmolimar commented Feb 20, 2019 •

edited by vcrfxia

Loading

ghost commented Feb 20, 2019

mmolimar commented Feb 20, 2019

ghost commented Feb 20, 2019

spena commented Feb 20, 2019

JimGalasyn left a comment

mmolimar commented Feb 21, 2019

hjafarpour left a comment

spena commented Mar 8, 2019

mmolimar commented Mar 15, 2019 •

edited

Loading

HLmohammed commented Apr 25, 2019 •

edited

Loading

JimGalasyn Apr 25, 2019 •

edited

Loading

mmolimar Apr 25, 2019

JimGalasyn left a comment

hjafarpour left a comment

hjafarpour Jun 18, 2019

big-andy-coates Jun 19, 2019 •

edited

Loading

big-andy-coates left a comment

big-andy-coates Jun 19, 2019 •

edited

Loading

mmolimar commented Jun 26, 2019

big-andy-coates commented Jun 27, 2019 •

edited

Loading

big-andy-coates left a comment

feat: New UNIX_TIMESTAMP and UNIX_DATE datetime functions #2459

feat: New UNIX_TIMESTAMP and UNIX_DATE datetime functions #2459

Conversation

mmolimar commented Feb 20, 2019 • edited by vcrfxia Loading

Description

Testing done

Reviewer checklist

ghost commented Feb 20, 2019

mmolimar commented Feb 20, 2019

ghost commented Feb 20, 2019

spena commented Feb 20, 2019

JimGalasyn left a comment

Choose a reason for hiding this comment

mmolimar commented Feb 21, 2019

hjafarpour left a comment

Choose a reason for hiding this comment

spena commented Mar 8, 2019

mmolimar commented Mar 15, 2019 • edited Loading

HLmohammed commented Apr 25, 2019 • edited Loading

JimGalasyn Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

mmolimar Apr 25, 2019

Choose a reason for hiding this comment

JimGalasyn left a comment

Choose a reason for hiding this comment

hjafarpour left a comment

Choose a reason for hiding this comment

hjafarpour Jun 18, 2019

Choose a reason for hiding this comment

big-andy-coates Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

mmolimar commented Jun 26, 2019

big-andy-coates commented Jun 27, 2019 • edited Loading

big-andy-coates left a comment

Choose a reason for hiding this comment

mmolimar commented Feb 20, 2019 •

edited by vcrfxia

Loading

mmolimar commented Mar 15, 2019 •

edited

Loading

HLmohammed commented Apr 25, 2019 •

edited

Loading

JimGalasyn Apr 25, 2019 •

edited

Loading

big-andy-coates Jun 19, 2019 •

edited

Loading

big-andy-coates Jun 19, 2019 •

edited

Loading

big-andy-coates commented Jun 27, 2019 •

edited

Loading