From 5def8c4679558ffb6922fd2406c670b4f26a95eb Mon Sep 17 00:00:00 2001 From: Andy Coates <8012398+big-andy-coates@users.noreply.github.com> Date: Thu, 16 Aug 2018 13:52:24 +0100 Subject: [PATCH] Switch the default mode of SUBSTRING to be the 'legacy' mode (#1735) Small improvements and additional tests for SUBSTRING --- docs/syntax-reference.rst | 2 +- .../io/confluent/ksql/util/KsqlConfig.java | 7 ++--- .../ksql/function/udf/string/Substring.java | 26 ++----------------- .../function/udf/string/SubstringTest.java | 11 +------- .../query-validation-tests/substring.json | 26 +++++++++++++++++-- 5 files changed, 32 insertions(+), 40 deletions(-) diff --git a/docs/syntax-reference.rst b/docs/syntax-reference.rst index e25284a59914..cdc92f5f4c5c 100644 --- a/docs/syntax-reference.rst +++ b/docs/syntax-reference.rst @@ -1099,7 +1099,7 @@ Scalar functions | | | It is possible to switch back to this legacy mode | | | | by setting | | | | ``ksql.functions.substring.legacy.args`` to | -| | | ``true`` | +| | | ``true``. Though this is not recommended. | +------------------------+------------------------------------------------------------+---------------------------------------------------+ | TIMESTAMPTOSTRING | ``TIMESTAMPTOSTRING(ROWTIME, 'yyyy-MM-dd HH:mm:ss.SSS')`` | Converts a BIGINT millisecond timestamp value into| | | | the string representation of the timestamp in | diff --git a/ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java b/ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java index da028f4de1f6..5c0150730f09 100644 --- a/ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java +++ b/ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java @@ -262,12 +262,13 @@ private static ConfigDef configDef(final boolean current) { + " calling System.exit or executing processes" ) .withClientSslSupport(); - for (final CompatibilityBreakingConfigDef compatiblityConfigDef + + for (final CompatibilityBreakingConfigDef compatibilityBreakingConfigDef : COMPATIBLY_BREAKING_CONFIG_DEBS) { if (current) { - compatiblityConfigDef.defineCurrent(configDef); + compatibilityBreakingConfigDef.defineCurrent(configDef); } else { - compatiblityConfigDef.defineOld(configDef); + compatibilityBreakingConfigDef.defineOld(configDef); } } return configDef; diff --git a/ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java b/ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java index a66932c4627b..35b33d8aa52f 100644 --- a/ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java +++ b/ksql-engine/src/main/java/io/confluent/ksql/function/udf/string/Substring.java @@ -17,7 +17,6 @@ package io.confluent.ksql.function.udf.string; import io.confluent.common.Configurable; -import io.confluent.common.config.ConfigException; import io.confluent.ksql.function.udf.Udf; import io.confluent.ksql.function.udf.UdfDescription; import io.confluent.ksql.function.udf.UdfParameter; @@ -40,8 +39,9 @@ public class Substring implements Configurable { @Override public void configure(final Map props) { + final KsqlConfig config = new KsqlConfig(props); final boolean legacyArgs = - getProps(props, KsqlConfig.KSQL_FUNCTIONS_SUBSTRING_LEGACY_ARGS_CONFIG, false); + config.getBoolean(KsqlConfig.KSQL_FUNCTIONS_SUBSTRING_LEGACY_ARGS_CONFIG); impl = legacyArgs ? new LegacyImpl() : new CurrentImpl(); } @@ -121,26 +121,4 @@ private static int getEndIndex(final String value, final int start, final int le return Math.max(Math.min(start + length, value.length()), start); } } - - @SuppressWarnings("SameParameterValue") - private static boolean getProps( - final Map props, - final String name, - final boolean defaultValue) { - - final Object value = props.get(name); - if (value == null) { - return defaultValue; - } - - if (value instanceof String) { - return Boolean.valueOf((String)value); - } - - if (!(value instanceof Boolean)) { - throw new ConfigException(name, value, "Value is not boolean"); - } - - return (Boolean)value; - } } \ No newline at end of file diff --git a/ksql-engine/src/test/java/io/confluent/ksql/function/udf/string/SubstringTest.java b/ksql-engine/src/test/java/io/confluent/ksql/function/udf/string/SubstringTest.java index 1b1fbf70aeb1..fb00610be40a 100644 --- a/ksql-engine/src/test/java/io/confluent/ksql/function/udf/string/SubstringTest.java +++ b/ksql-engine/src/test/java/io/confluent/ksql/function/udf/string/SubstringTest.java @@ -21,8 +21,8 @@ import static org.junit.Assert.assertThat; import com.google.common.collect.ImmutableMap; -import io.confluent.common.config.ConfigException; import io.confluent.ksql.util.KsqlConfig; +import org.apache.kafka.common.config.ConfigException; import org.junit.Before; import org.junit.Test; @@ -215,15 +215,6 @@ public void shouldNotEnterLegacyModeWithFalseBooleanConfig() { assertThat(udfIsInLegacyMode(), is(false)); } - @Test - public void shouldNotEnterLegacyModeWithOtherStringConfig() { - // When: - configure("what ever"); - - // Then: - assertThat(udfIsInLegacyMode(), is(false)); - } - @Test(expected = ConfigException.class) public void shouldThrowOnInvalidLegacyModeValueType() { configure(1.0); diff --git a/ksql-engine/src/test/resources/query-validation-tests/substring.json b/ksql-engine/src/test/resources/query-validation-tests/substring.json index 9c34d1c3a1d5..b2a0668403ce 100644 --- a/ksql-engine/src/test/resources/query-validation-tests/substring.json +++ b/ksql-engine/src/test/resources/query-validation-tests/substring.json @@ -1,11 +1,17 @@ { "comments": [ - "Tests covering the use of the SUBSTRING function" + "Tests covering the use of the SUBSTRING function.", + "NOTE: At present the SUBSTRING function has two modes: current and legacy.", + " v5.0 and earlier only have legacy mode.", + " v5.1 and later have both current and legacy. New queries default to the current mode" ], "tests": [ { "name": "do substring with just pos", "format": ["JSON"], + "properties": { + "ksql.functions.substring.legacy.args": false + }, "statements": [ "CREATE STREAM TEST (source VARCHAR) WITH (kafka_topic='test_topic', value_format='JSON');", "CREATE STREAM OUTPUT AS SELECT SUBSTRING(source, 6) AS SUBSTRING, SUBSTRING(null, 1) AS NULL_STR, SUBSTRING(source, null) AS NULL_POS FROM TEST;" @@ -24,7 +30,9 @@ { "name": "do substring with pos and length", "format": ["JSON"], - "statements": [ + "properties": { + "ksql.functions.substring.legacy.args": false + },"statements": [ "CREATE STREAM TEST (source VARCHAR) WITH (kafka_topic='test_topic', value_format='JSON');", "CREATE STREAM OUTPUT AS SELECT SUBSTRING(source, 6, 3) AS SUBSTRING, SUBSTRING(null, 1) AS NULL_STR, SUBSTRING(source, null) AS NULL_POS, SUBSTRING(source, 6, null) AS NULL_LEN FROM TEST;" ], @@ -80,6 +88,20 @@ {"topic": "OUTPUT", "key": 2, "value": {"SUBSTRING":"nothe", "NULL_STR":null, "NULL_START":null, "NULL_END":null}, "timestamp": 0}, {"topic": "OUTPUT", "key": 2, "value": {"SUBSTRING":null, "NULL_STR":null, "NULL_START":null, "NULL_END":null}, "timestamp": 0} ] + }, + { + "name": "should default to current mode for new queries", + "format": ["JSON"], + "statements": [ + "CREATE STREAM TEST (source VARCHAR) WITH (kafka_topic='test_topic', value_format='JSON');", + "CREATE STREAM OUTPUT AS SELECT SUBSTRING(source, 6) AS SUBSTRING FROM TEST;" + ], + "inputs": [ + {"topic": "test_topic", "key": 1, "value": {"source": "some_string"}, "timestamp": 0} + ], + "outputs": [ + {"topic": "OUTPUT", "key": 1, "value": {"SUBSTRING":"string"}, "timestamp": 0} + ] } ] } \ No newline at end of file