feat(migrations): support DEFINE and UNDEFINE statements #7366

jzaralim · 2021-04-10T05:28:41Z

Description

^^^

Previously, the tool parsed each command, collected them and then executed them. Because the variables are tracked in the java client, but SET/UNSET/INSERT VALUES don't do variable substitution through the client, the flow was refactored to parse commands and then execute them one by one.

Testing done

unit + integration

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

jzaralim · 2021-04-10T05:29:22Z

ksqldb-tools/src/test/java/io/confluent/ksql/tools/migrations/MigrationsTest.java

-            "CREATE STREAM HOMES (ADDR ADDRESS) WITH (KAFKA_TOPIC='HOMES', PARTITIONS=1, VALUE_FORMAT='JSON');" +
-            "INSERT INTO HOMES VALUES (STRUCT(number := 123, street := 'sesame st', city := 'New York City'));" +
+            "DEFINE variable = 'HOMES';" +
+            "CREATE STREAM ${variable} (ADDR ADDRESS) WITH (KAFKA_TOPIC='${variable}', PARTITIONS=1, VALUE_FORMAT='JSON');" +


This line passing depends on #7365

vcrfxia

Thanks @jzaralim -- this is exciting! Comments inline. Also I updated the PR title to reflect that this PR adds support for DEFINE and UNDEFINE statements in the migrations tool, but doesn't add the full variable substitution support (i.e., the --define option) specified in the KLIP.

docs/operate-and-deploy/migrations-tool.md

vcrfxia · 2021-04-13T21:20:49Z

...b-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/ApplyMigrationCommand.java

@@ -312,13 +314,17 @@ private void executeCommands(
      final String previous
  ) {
    final Map<String, Object> properties = new HashMap<>();
-    final List<SqlCommand> commands = CommandParser.parse(migrationFileContent);


Ideally we would still parse everything before starting to execute any commands, to avoid situations where a migrations file is partially executed because a later statement fails to parse. As discussed offline, we can achieve this by having a validation loop that is identical to the execution loop except only DEFINE and UNDEFINE statements are executed (i.e., sent to the java client for purposes of tracking variables, without anything actually being sent to the ksqlDB server).

...b-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/ApplyMigrationCommand.java

vcrfxia · 2021-04-13T21:21:57Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

@@ -56,6 +57,10 @@
      Pattern.compile("\\s*UNSET\\s+'((?:[^']*|(?:''))*)'\\s*;\\s*");
  private static final Pattern DROP_CONNECTOR =
      Pattern.compile("\\s*DROP\\s+CONNECTOR\\s+(.*?)\\s*;\\s*");
+  private static final Pattern DEFINE_VARIABLE = Pattern.compile(
+      "\\s*DEFINE\\s+([a-zA-Z_][a-zA-Z0-9_@]*)\\s*=\\s*'((?:[^']*|(?:''))*)'\\s*;\\s*");


Just checking: DEFINE statements require that the values be strings, even though the java client and HTTP endpoints allow non-string values, right? It looks like the values are converted into strings by SessionProperties anyway. Is there a reason we didn't have the HTTP endpoints require string values to keep things simpler/more consistent? It's worth considering whether the java client should only accept string values as well.

It follows the same pattern as SET, which also requires strings in the statement but can be passed as object parameters. I can see two reasons this could be convenient:

Users using the endpoints don't need to treat sessionProperties and sessionVariables differently

Client users can make requests like client.define("name", 5); or client.define("name", false); without having to convert them to strings.

These are pretty minor reasons though. Swapping everything to strings is a pretty easy change.

That makes sense. Let's leave this as is!

vcrfxia · 2021-04-13T21:22:32Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

+  private static final Pattern DEFINE_VARIABLE = Pattern.compile(
+      "\\s*DEFINE\\s+([a-zA-Z_][a-zA-Z0-9_@]*)\\s*=\\s*'((?:[^']*|(?:''))*)'\\s*;\\s*");
+  private static final Pattern UNDEFINE_VARIABLE =
+      Pattern.compile("\\s*UNDEFINE\\s+([a-zA-Z_][a-zA-Z0-9_@]*)\\s*;\\s*");


Where did this set of valid characters in the variable name (i.e., [a-zA-Z_][a-zA-Z0-9_@]*) come from?

The variable name follows the pattern for identifiers in the parser:

ksql/ksqldb-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4

Line 582 in 01bb840

: (LETTER | '_') (LETTER | DIGIT | '_' | '@' )*

OK. It's not great that we've got it in two places now (the parser and also the migrations tool) but there are disadvantages to having them be shared as well. How about we add a comment to this line in the migrations tool code to reference the SqlBase.g4 line (with a git permalink) so if the parser is changed in the future and something breaks, we'll at least know where to look to update this.

vcrfxia · 2021-04-13T21:23:58Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

-        return getUnsetPropertyCommand(sql);
+        return getUnsetPropertyCommand(sql, variables);
+      case DEFINE_VARIABLE:
+        return getDefineVariableCommand(sql);


This also needs to support variable substitution, right? In case users want to define a variable based on other variables.

I'm not sure whether it makes sense to support variable substitution for the variable name in addition to for the variable value. It'd be good to check what the ksqlDB CLI's stance on this is, and add support if needed.

Updated. Only the variable values can be replaced.

vcrfxia · 2021-04-13T21:27:19Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

+  }
+
+  private static SqlDefineVariableCommand getDefineVariableCommand(final String sql) {
+    final Matcher defineVariableMatcher = DEFINE_VARIABLE.matcher(sql);


Haven't tried this so I might be mistaken but I think we've got a bug around casing here: transformToSqlCommand() uppercases the sql before applying the regex matchers but we don't do that here (we can't because we don't want to strings or quoted identifiers), which means this matcher might fail to find a match (e.g., if the user lowercases DEFINE in their sql command). I think this bug also affects the other statement types.

If this is true, we definitely need to fix this, and we should add test coverage for it too.

Added a fix

Thanks. Do you know whether this is a problem for 0.17 (e.g., for INSERT VALUES)? If so, we should backport a fix to 6.2.x (requires going through the CP blocker process). We should also consider getting a fix into 0.17 but I think that's unlikely at this point, in which case we'd want to document the limitation.

It affects SET and UNSET. Filed a blocker, but IMO because there is a clear workaround (tell users to capitalize SET and UNSET), it's not terrible if it doesn't get included.

Agreed it's not terrible if it's not included but I think it's still better to fix it before the 6.2 release. 0.17 probably isn't worth it, though.

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

ksqldb-tools/src/test/java/io/confluent/ksql/tools/migrations/util/CommandParserTest.java

…ration

vcrfxia

Thanks @jzaralim -- looking good! A couple important questions but nothing major in terms of changes. Looking forward to getting this merged :)

docs/operate-and-deploy/migrations-tool.md

...b-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/ApplyMigrationCommand.java

vcrfxia · 2021-04-15T15:38:49Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

+  }
+
+  private static SqlDefineVariableCommand getDefineVariableCommand(final String sql) {
+    final Matcher defineVariableMatcher = DEFINE_VARIABLE.matcher(sql);


Thanks. Do you know whether this is a problem for 0.17 (e.g., for INSERT VALUES)? If so, we should backport a fix to 6.2.x (requires going through the CP blocker process). We should also consider getting a fix into 0.17 but I think that's unlikely at this point, in which case we'd want to document the limitation.

vcrfxia · 2021-04-15T15:40:36Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

+      "\\s*DEFINE\\s+([a-zA-Z_][a-zA-Z0-9_@]*)\\s*=\\s*'((?:[^']*|(?:''))*)'\\s*;\\s*",
+      Pattern.CASE_INSENSITIVE);
+  private static final Pattern UNDEFINE_VARIABLE = Pattern.compile(
+      "\\s*UNDEFINE\\s+([a-zA-Z_][a-zA-Z0-9_@]*)\\s*;\\s*", Pattern.CASE_INSENSITIVE);


Do we still need the toUpperCase() call in transformToSqlCommand() now that we have case-insensitivity?

Yup, it's used to determine the type of command

But why do we need to uppercase the command if the regex is case-insensitive now?

The regexes are used to determine things like the name of a connector being dropped. There is some logic before that for determining the type of command or if the command is supported, which works by finding specific keywords in the sql string.

Ah, got it. Would it be equivalent if we removed the toUpperCase() and instead replaced the .equals() for token comparison with .equalsIgnoreCase() instead? It's mildly less efficient but I think it improves readability by not separating the logic for case-insensitive comparison across two different places.

There are some places where it uses contains instead of equals, for example tokens.contains(VALUES), so removing the toUpperCase would actually make it more confusing. I've added some comments to clarify what tokens is supposed to be.

Sounds good. Thanks!

...ols/src/test/java/io/confluent/ksql/tools/migrations/commands/ApplyMigrationCommandTest.java

vcrfxia

LGTM! Thanks @jzaralim .

...b-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/ApplyMigrationCommand.java

vcrfxia · 2021-04-15T21:38:28Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/CommandParser.java

+      "\\s*DEFINE\\s+([a-zA-Z_][a-zA-Z0-9_@]*)\\s*=\\s*'((?:[^']*|(?:''))*)'\\s*;\\s*",
+      Pattern.CASE_INSENSITIVE);
+  private static final Pattern UNDEFINE_VARIABLE = Pattern.compile(
+      "\\s*UNDEFINE\\s+([a-zA-Z_][a-zA-Z0-9_@]*)\\s*;\\s*", Pattern.CASE_INSENSITIVE);


Ah, got it. Would it be equivalent if we removed the toUpperCase() and instead replaced the .equals() for token comparison with .equalsIgnoreCase() instead? It's mildly less efficient but I think it improves readability by not separating the logic for case-insensitive comparison across two different places.

...b-tools/src/main/java/io/confluent/ksql/tools/migrations/commands/ApplyMigrationCommand.java

feat: enable variable substitution for migrations tool

bdaeb93

jzaralim requested review from JimGalasyn and a team as code owners April 10, 2021 05:28

jzaralim commented Apr 10, 2021

View reviewed changes

JimGalasyn approved these changes Apr 12, 2021

View reviewed changes

vcrfxia reviewed Apr 13, 2021

View reviewed changes

vcrfxia changed the title ~~feat: enable variable substitution for migrations tool~~ feat(migrations): support DEFINE and UNDEFINE statements Apr 13, 2021

Zara Lim added 3 commits April 13, 2021 16:10

Merge branch 'master' of github.com:confluentinc/ksql into varsub-mig…

1dcb6b2

…ration

address review comments

81d55ce

checkstyle

19c7637

vcrfxia reviewed Apr 15, 2021

View reviewed changes

address review comments

25973a3

vcrfxia approved these changes Apr 15, 2021

View reviewed changes

address review comments

e1aaf8a

jzaralim mentioned this pull request Apr 20, 2021

feat(migrations): add --define flag to apply command #7401

Merged

2 tasks

jzaralim merged commit 330db93 into confluentinc:master Apr 21, 2021

jzaralim deleted the varsub-migration branch April 21, 2021 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(migrations): support DEFINE and UNDEFINE statements #7366

feat(migrations): support DEFINE and UNDEFINE statements #7366

jzaralim commented Apr 10, 2021

jzaralim Apr 10, 2021

vcrfxia left a comment

vcrfxia Apr 13, 2021

vcrfxia Apr 13, 2021

jzaralim Apr 13, 2021

vcrfxia Apr 15, 2021

vcrfxia Apr 13, 2021

jzaralim Apr 13, 2021

vcrfxia Apr 15, 2021

vcrfxia Apr 13, 2021

jzaralim Apr 14, 2021

vcrfxia Apr 13, 2021

jzaralim Apr 14, 2021

vcrfxia Apr 15, 2021

jzaralim Apr 15, 2021

vcrfxia Apr 15, 2021

vcrfxia left a comment

vcrfxia Apr 15, 2021

vcrfxia Apr 15, 2021

jzaralim Apr 15, 2021

vcrfxia Apr 15, 2021

jzaralim Apr 15, 2021

vcrfxia Apr 15, 2021

jzaralim Apr 15, 2021

vcrfxia Apr 15, 2021

vcrfxia left a comment

vcrfxia Apr 15, 2021

feat(migrations): support DEFINE and UNDEFINE statements #7366

feat(migrations): support DEFINE and UNDEFINE statements #7366

Conversation

jzaralim commented Apr 10, 2021

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

vcrfxia 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

vcrfxia 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

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment