Added stringtodate and datetostring UDFs #1890

andybryant · 2018-09-14T03:19:16Z

Description

Added stringtodate and datetostring UDFs to convert to and from a formatted date string and an integer representing days since epoch. This format is used by Kafka Connect to represent Date objects with no time or time zone component.

In a separate commit on the same PR I also updated stringtotimestamp and timestamptostring to use the new UDF api and to support dynamically supplied format patterns.

Testing done

Added tests for all new functionality and ran locally. Generated and views updated docs locally.

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 · 2018-09-14T03:19:17Z

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

Always at your service,

clabot

JimGalasyn

LGTM!

andybryant · 2018-09-27T05:02:14Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

@@ -375,7 +375,7 @@ public void shouldListFunctions() {

    // not going to check every function
    assertThat(functionList.getFunctions(), hasItems(
-        new SimpleFunctionInfo("TIMESTAMPTOSTRING", FunctionType.scalar),
+        new SimpleFunctionInfo("EXTRACTJSONFIELD", FunctionType.scalar),


Functions that use the new Udf annotation don't show up in this list. Changing the test to use another Kudf based function.

big-andy-coates

Thanks @andybryant, this is really cool that you've taken the time to raise this.

Sorry for the delay in getting it reviewed.

I've left a few comments and suggestions. Take a look and see what you think.

Thanks,

Andy

big-andy-coates · 2018-10-01T13:48:30Z

docs/developer-guide/syntax-reference.rst

@@ -1071,6 +1071,14 @@ Scalar functions
 +------------------------+------------------------------------------------------------+---------------------------------------------------+
 | CONCAT                 |  ``CONCAT(col1, '_hello')``                                | Concatenate two strings.                          |
 +------------------------+------------------------------------------------------------+---------------------------------------------------+
+| DATETOSTRING           |  ``DATETOSTRING(START_DATE, 'yyyy-MM-dd')``                | Converts an INT date value into                   |


"Converts an integer representation of a date into..."

big-andy-coates · 2018-10-01T13:48:54Z

docs/developer-guide/syntax-reference.rst

@@ -1140,6 +1148,12 @@ Scalar functions
 +------------------------+------------------------------------------------------------+---------------------------------------------------+
 | ROUND                  |  ``ROUND(col1)``                                           | Round a value to the nearest BIGINT value.        |
 +------------------------+------------------------------------------------------------+---------------------------------------------------+
+| STRINGTODATE           |  ``STRINGTODATE(col1, 'yyyy-MM-dd')``                      | Converts a string value in the given              |


"Converts a string representation of a date in..."

" into an integer representing the days since epoch."

big-andy-coates · 2018-10-01T13:51:09Z

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

 import io.confluent.ksql.util.timestamp.StringToTimestampParser;

-public class StringToTimestamp implements Kudf {
+@UdfDescription(name = "stringtotimestamp", author = "Confluent",


"Converts a string representation of a date ..."

(You're wording on the new UDFs is much better than the wording on the old ones!)

big-andy-coates · 2018-10-01T13:51:19Z

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

-    if (timestampParser == null) {
-      timestampParser = new StringToTimestampParser(args[1].toString());
-    }
+  @Udf(description = "Converts a string value in the given format into the BIGINT value"


"Converts a string representation of a date..."

big-andy-coates · 2018-10-01T13:52:54Z

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

+      + " days since epoch using the given format pattern."
+      + " The format pattern should be in the format expected by"
+      + " java.time.format.DateTimeFormatter")
+  public int stringToDate(final String formattedDate, final String formatPattern) {


Could you look to make use of @UdfParameter on all of your UDF params please. The actual names of the fields are not currently used.

Switched to using @UdfParameter for all 4 UDFs

big-andy-coates · 2018-10-01T14:20:58Z

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

  @Test
  public void shouldThrowIfFormatInvalid() {
-    expectedException.expect(KsqlFunctionException.class);
+    expectedException.expect(UncheckedExecutionException.class);


exception type again.

big-andy-coates · 2018-10-01T14:21:16Z

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

  }

  @Test
-  public void shouldBeThreadSafe() {
+  public void shouldWorkWithManyDifferentFormatters() {


combine with above again.

big-andy-coates · 2018-10-01T14:22:00Z

ksql-engine/src/test/resources/query-validation-tests/stringdate.json

+        "CREATE STREAM TS AS select id, stringtodate(date, format) as ts from test;"
+      ],
+      "inputs": [
+        {"topic": "test_topic", "key": 0, "value": "0,zero,2018-05-11,yyyy-MM-dd", "timestamp": 0},


can we add a literal in one of these formats please?

big-andy-coates · 2018-10-01T14:22:19Z

ksql-engine/src/test/resources/query-validation-tests/stringdate.json

@@ -0,0 +1,31 @@
+{
+  "comments": [
+    "You can specify multiple statements per test case, i.e., to set up the various streams needed",


drop this cut & paste comments section if you don't mind.

done (in both json files)

big-andy-coates · 2018-10-01T14:22:26Z

ksql-engine/src/test/resources/query-validation-tests/datestring.json

+        "CREATE STREAM DATE_STREAM AS select ID, datetostring(START_DATE, DATE_FORMAT) as CUSTOM_FORMATTED_START_DATE from test;"
+      ],
+      "inputs": [
+        {"topic": "test_topic", "key": 1, "value": {"ID": 1, "START_DATE": 17662, "DATE_FORMAT": "yyyy-MM-dd"}, "timestamp": 0},


can we add a literal in one of these formats please?

the last one is literal

andybryant · 2018-10-04T00:52:51Z

Hi @big-andy-coates

Thanks for the feedback - lots of good ideas there. I've pushed another commit that I think addresses all your comments. I kept the 3 separate commits for now to make reviewing easier, but can squash them down to one commit once your happy with it.

Cheers
Andy

big-andy-coates

Thanks @andybryant LGTM.

Sorry for the delay in reviewing!

Just need another engineer to double check. I'll look to get this cherry picked into 5.1. release.

big-andy-coates · 2018-10-10T12:48:09Z

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

+  public String dateToString(
+      @UdfParameter(value = "epochDays",
+          description = "The Epoch Day to convert,"
+              + " based on the epoch 1970-01-01") final Integer epochDays,


maybe use int rather than Integer as it'll throw an NPE on null anyway.

dguy

Thanks @andybryant for submitting this PR! I've left one question, but otherwise LGTM

dguy · 2018-10-10T12:49:16Z

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

+      final Timestamp timestamp = new Timestamp(epochMilli);
+      final DateTimeFormatter formatter = formatters.get(formatPattern);
+      return timestamp.toInstant()
+          .atZone(ZoneId.systemDefault())


Do we want this to use the system timezone? i.e., dates are all UTC in KSQL. So this is deviating from the norm. if this is indeed the case that we want this, then can you update the description etc to make it clear what is happening?

The old StringToTimestamp implementation used system default time zone when no time zone was provided. I can change to UTC and update the docs, but wouldn't this be considered a breaking change?

I've assumed we want to keep the old behaviour for now and have updated the docs accordingly.

andybryant · 2018-10-11T05:46:58Z

Fixed up docs based on @dguy feedback and switched DateToString to use int for epochDays.

dguy · 2018-10-11T14:38:22Z

Thanks @andybryant can you please merge with the latest master. Assuming it passes we can then merge it. Thanks for your patience

confluentinc#1851) Added stringtodate and datetostring UDFs (confluentinc#1851)

andybryant · 2018-10-14T10:31:45Z

@dguy - updated to latest master. Cheers Andy

dguy · 2018-10-15T11:02:21Z

Thanks for the contribution @andybryant!

andybryant · 2018-10-16T02:11:10Z

Awesome - thanks @dguy
@big-andy-coates - created a separate PR #2056 with the two commits cherry-picked for the 5.1.x branch in case there's still a possibility of getting it into that release

andybryant requested review from JimGalasyn and a team as code owners September 14, 2018 03:19

andybryant mentioned this pull request Sep 14, 2018

[Enhancement] Add standard UDF for converting Kafka Connect Dates to/from Strings #1851

Closed

andybryant force-pushed the feature-datetostring-udf branch 3 times, most recently from 6fa7979 to c07fdb5 Compare September 14, 2018 05:59

JimGalasyn approved these changes Sep 14, 2018

View reviewed changes

andybryant force-pushed the feature-datetostring-udf branch from c07fdb5 to 35c3507 Compare September 16, 2018 23:33

andybryant force-pushed the feature-datetostring-udf branch 3 times, most recently from adb5cc0 to 780fae4 Compare September 27, 2018 04:54

andybryant commented Sep 27, 2018

View reviewed changes

andybryant force-pushed the feature-datetostring-udf branch from 780fae4 to 1c4291a Compare September 27, 2018 06:23

big-andy-coates reviewed Oct 1, 2018

View reviewed changes

andybryant force-pushed the feature-datetostring-udf branch from 1c4291a to bea71d0 Compare October 4, 2018 00:49

andybryant force-pushed the feature-datetostring-udf branch 3 times, most recently from da8d37e to d999593 Compare October 4, 2018 01:35

big-andy-coates approved these changes Oct 10, 2018

View reviewed changes

dguy approved these changes Oct 10, 2018

View reviewed changes

andybryant force-pushed the feature-datetostring-udf branch 3 times, most recently from e49e2dc to c5df215 Compare October 11, 2018 05:43

Andy Bryant added 2 commits October 14, 2018 20:33

Added stringtodate and datetostring UDFs (confluentinc#1851)

5d40f1b

Converted stringtotimestamp and timestamptostring to latest UDF format (

0b98da1

confluentinc#1851) Added stringtodate and datetostring UDFs (confluentinc#1851)

andybryant force-pushed the feature-datetostring-udf branch from c5df215 to 0b98da1 Compare October 14, 2018 09:34

dguy merged commit 2ff99fa into confluentinc:master Oct 15, 2018

andybryant mentioned this pull request Oct 16, 2018

Added stringtodate and datetostring UDFs for 5.1.x #2056

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added stringtodate and datetostring UDFs #1890

Added stringtodate and datetostring UDFs #1890

andybryant commented Sep 14, 2018

ghost commented Sep 14, 2018

JimGalasyn left a comment

andybryant Sep 27, 2018

big-andy-coates left a comment

big-andy-coates Oct 1, 2018

big-andy-coates Oct 1, 2018

big-andy-coates Oct 1, 2018

big-andy-coates Oct 1, 2018

big-andy-coates Oct 1, 2018

andybryant Oct 4, 2018

big-andy-coates Oct 1, 2018

big-andy-coates Oct 1, 2018

big-andy-coates Oct 1, 2018

andybryant Oct 4, 2018

big-andy-coates Oct 1, 2018

andybryant Oct 4, 2018

big-andy-coates Oct 1, 2018

andybryant Oct 4, 2018

andybryant commented Oct 4, 2018 •

edited

Loading

big-andy-coates left a comment

big-andy-coates Oct 10, 2018

andybryant Oct 11, 2018

dguy left a comment

dguy Oct 10, 2018

andybryant Oct 11, 2018

andybryant Oct 11, 2018

andybryant commented Oct 11, 2018

dguy commented Oct 11, 2018

andybryant commented Oct 14, 2018

dguy commented Oct 15, 2018

andybryant commented Oct 16, 2018

Added stringtodate and datetostring UDFs #1890

Added stringtodate and datetostring UDFs #1890

Conversation

andybryant commented Sep 14, 2018

Description

Testing done

Reviewer checklist

ghost commented Sep 14, 2018

JimGalasyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybryant commented Oct 4, 2018 • edited Loading

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybryant commented Oct 11, 2018

dguy commented Oct 11, 2018

andybryant commented Oct 14, 2018

dguy commented Oct 15, 2018

andybryant commented Oct 16, 2018

andybryant commented Oct 4, 2018 •

edited

Loading