More SQL-like Substring #1635

big-andy-coates · 2018-07-26T09:10:29Z

Description

Switch SUBSTRING from SUBSTRING(str, startIndex, endIndex) with zero-based indexing to SUBSTRING(str, pos, len) with:

one-based indexing
returns null for any null parameters
ignores out of bound indexes, (i.e. treats them as being just inbound)
as used by other SQL providers.

Uses can set ksql.functions.substring.legacy.args to true, (either through server properties or session properties, to switch the implementation back to legacy mode, i.e. SUBSTRING(str, startIndex, endIndex) and error handling that throws a lot of exceptions.

Fixes #1634

Because I've extended the description of SubString, I've also improved the formatting of the output of the DESCRIBE FUNCTION x; in the CLI, so that long descriptions are correctly split across lines and properly indented. New output looks like:

ksql> describe function substring;

Name        : SUBSTRING
Author      : Confluent
Overview    : Returns a substring of the passed in value.
              The behaviour of this function changed in release 5.1. It is 
              possible to switch the function back to pre-v5.1 functionality via
               the setting:
              	ksql.functions.substring.legacy.args
              This can be set globally, through the server configuration file, 
              or per sessions or query via the set command.
Type        : scalar
Jar         : internal
Variations  : 

  Variation   : SUBSTRING(value VARCHAR, pos INT, len INT)
  Returns     : VARCHAR
  Description : Returns a substring of str that starts at pos and is of length 
                len
  value       : The source string. If null, then function returns null.
  pos         : The base-one position the substring starts from. If null, then 
                function returns null. (If in legacy mode, this argument is 
                base-zero)
  len         : The length of the substring to extract. If null, then function 
                returns null. (If in legacy mode, this argument is the endIndex 
                (exclusive), rather than the length

  Variation   : SUBSTRING(value VARCHAR, pos INT)
  Returns     : VARCHAR
  Description : Returns a substring of str that starts at pos  and continues to 
                the end of the string
  value       : The source string. If null, then function returns null.
  pos         : The base-one position the substring starts from. If null, then 
                function returns null. (If in legacy mode, this argument is 
                base-zero)

Related release notes added in this PR: https://github.com/confluentinc/docs/pull/1095

Testing done

Unit tests added and JSON based functional test.

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 #")

Fixes confluentinc#1634

dguy · 2018-07-26T09:38:10Z

This breaks compatibility with 4.1.x substring function

big-andy-coates · 2018-07-26T10:24:07Z

This breaks compatibility with 4.1.x substring function

Well, good that its not a release issue. Bad that our SUBSTRING is different to everyone else :-/

hjafarpour

LGTM.

hjafarpour · 2018-07-26T17:38:16Z

ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java

-  public String substring(final String value, final int startIndex) {
-    return value.substring(startIndex);
+  @Udf(description = "Returns a substring of str that starts at pos "
+                     + "and continues to the end of the string")


Let's also document that the pos starts from 1 not 0.

blueedgenick · 2018-07-26T17:44:33Z

ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java

-      + " extends to the character at endIndex -1.")
-  public String substring(final String value, final int startIndex, final int endIndex) {
-    return value.substring(startIndex, endIndex);
+  @Udf(description = "Returns a substring of str that starts at pos and is of length len")


was expecting to see some shiny new @UdfParameter decoration here to help teh user know which arg is the length and which one the startPos ? :-)

Different branch. That's only available in master. This is targeted at 5.0. Though now its not release related we can change this to master.

Now on master branch - so annotations added!

# Conflicts: # ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java

…s can control via the configuration `ksql.functions.substring.legacy.args`.

rodesai · 2018-08-02T07:36:44Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

@@ -187,6 +200,12 @@ private static ConfigDef configDef(final boolean current) {
            "",
            ConfigDef.Importance.LOW,
            KSQL_OUTPUT_TOPIC_NAME_PREFIX_DOCS
+        ).define(


You can add this to COMPATIBILITY_BREAKING_CONFIG_DEFS on line 119 instead and set the write default to True and the read default to False. That way new queries can use the new SUBSTR behavior, and any existing queries will use the old behaviour.

Oh, cool. Not noticed that before.

I've added it to COMPATIBILITY_BREAKING_CONFIG_DEFS and removed it from the main def list.

rodesai · 2018-08-02T07:42:44Z

ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java

+  @Override
+  public void configure(final Map<String, ?> props) {
+    final boolean legacyArgs =
+        getProps(props, KSQ_FUNCTIONS_PROPERTY_PREFIX + "substring.legacy.args", false);


Why not use KSQL_FUNCTIONS_SUBSRTRING_LEGACY_ARGS_CONFIG here?

rodesai · 2018-08-02T07:50:29Z

ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java

+    }
+
+    private static int getStartIndex(final String value, final Integer pos) {
+      return pos < 0


If pos is 0 the substr call will throw an IndexOutOfBoundsException. Is that what we want?

Good spot! Test case added and code fixed.

big-andy-coates · 2018-08-02T13:03:35Z

@rodesai thanks for the review! I've pushed some changes to address your comments.

rodesai

LGTM!

…y, so as not to be a breaking change. Original code change is in confluentinc#1635.

More SQL-like SUBSTRING

c9edc24

Fixes confluentinc#1634

big-andy-coates requested review from dguy, rmoff and a team July 26, 2018 09:10

Fix checkstyle.

3ababf4

hjafarpour approved these changes Jul 26, 2018

View reviewed changes

blueedgenick reviewed Jul 26, 2018

View reviewed changes

big-andy-coates changed the base branch from 5.0.x to master July 30, 2018 13:39

big-andy-coates added 4 commits August 1, 2018 13:53

Merge branch 'master' into substring

0312262

# Conflicts: # ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java

Merge branch 'master' into substring

0255ef4

SUBSTRING is now more SQL compliant. It has a legacy mode, which user…

aa3952e

…s can control via the configuration `ksql.functions.substring.legacy.args`.

Fix tests

3d2892a

big-andy-coates mentioned this pull request Aug 1, 2018

KSQL Functions and Arrays should use 1-based indexing #1659

Closed

big-andy-coates added 4 commits August 1, 2018 18:18

fix integration test

bc590b5

Fix tests and fix formatting issues.

36f3dff

Doc updates.

8312ba1

Merge branch 'master' into substring

30936c8

big-andy-coates requested a review from a team August 1, 2018 23:48

rodesai reviewed Aug 2, 2018

View reviewed changes

Updates, including Rohan's requested changes.

b3b1707

Merge branch 'master' into substring

b0fca1f

rodesai approved these changes Aug 3, 2018

View reviewed changes

big-andy-coates merged commit 5455868 into confluentinc:master Aug 3, 2018

big-andy-coates deleted the substring branch August 3, 2018 12:26

This was referenced Aug 14, 2018

Changing the array behavior to start the index from 1. #1682

Closed

Switch defaults for SUBSTRING and ARRAY access #1734

Closed

big-andy-coates added a commit to big-andy-coates/ksql that referenced this pull request Aug 14, 2018

Switch the default mode of SUBSTRING to be the 'legacy' mode initiall…

6865965

…y, so as not to be a breaking change. Original code change is in confluentinc#1635.

big-andy-coates mentioned this pull request Aug 14, 2018

Switch the default mode of SUBSTRING to be the 'legacy' mode #1735

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More SQL-like Substring #1635

More SQL-like Substring #1635

big-andy-coates commented Jul 26, 2018 •

edited

Loading

dguy commented Jul 26, 2018

big-andy-coates commented Jul 26, 2018

hjafarpour left a comment

hjafarpour Jul 26, 2018

blueedgenick Jul 26, 2018

big-andy-coates Jul 27, 2018

big-andy-coates Aug 1, 2018

rodesai Aug 2, 2018

big-andy-coates Aug 2, 2018

rodesai Aug 2, 2018

big-andy-coates Aug 2, 2018

rodesai Aug 2, 2018

big-andy-coates Aug 2, 2018

big-andy-coates commented Aug 2, 2018

rodesai left a comment

More SQL-like Substring #1635

More SQL-like Substring #1635

Conversation

big-andy-coates commented Jul 26, 2018 • edited Loading

Description

Testing done

Reviewer checklist

dguy commented Jul 26, 2018

big-andy-coates commented Jul 26, 2018

hjafarpour 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

big-andy-coates commented Aug 2, 2018

rodesai left a comment

Choose a reason for hiding this comment

big-andy-coates commented Jul 26, 2018 •

edited

Loading