feat: drop legacy key field functionality #3764

big-andy-coates · 2019-11-05T23:05:06Z

Description

BREAKING CHANGE

This drops functionality that maintained a legacy version of the key field.
This legacy version was often wrong and resulted in unnecessary repartition steps being performed.
The functionality was previously required to ensure existing legacy queries continued to have the unnecessary repartitions to ensure no data loss.

With a major version bump this can now be dropped.

Reviewing notes:

Start with the changes to KeyField as this is the crux of the change. The legacy field is no more! Most of the rest of the code changes is removing the legacy field.
JoinNode has received a little bit of love to tidy things up. The correct keyField is now constructed in the constructor and used for all joins later on.
KsqlStructuredDataOutputNode has been simplified with the removal of SchemaKStream.withKeyField.

Testing done

Usual.

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

BREAKING CHANGE This drops functionality that maintained a legacy version of the key field. This legacy version was often wrong and resulted in unnecessary repartition steps being performed. The functionality was previously required to ensure existing legacy queries continued to have the unnecessary repartitions to ensure no data loss. With a major version bump this can now be dropped.

big-andy-coates · 2019-11-05T23:08:25Z

ksql-engine/src/main/java/io/confluent/ksql/planner/plan/JoinNode.java

@@ -281,12 +283,6 @@ Joiner getJoiner(final DataSourceType leftType,
        final ColumnRef joinFieldName,
        final Stacker contextStacker
    ) {
-      final LogicalSchema schema = stream.getSchema();
-
-      schema.findValueColumn(joinFieldName)


removed this test as its duplicates one inside selectKey.

big-andy-coates · 2019-11-05T23:08:51Z

ksql-engine/src/main/java/io/confluent/ksql/planner/plan/JoinNode.java

-     * @param leftKeyField the key field of the left source.
-     * @return the key field that should be used by the resultant joined stream.
-     */
-    static KeyField getJoinedKeyField(final SourceName leftAlias, final KeyField leftKeyField) {


no longer needed as the correct keyfield is built in the constructor.

big-andy-coates · 2019-11-05T23:08:59Z

ksql-engine/src/main/java/io/confluent/ksql/planner/plan/JoinNode.java

-     * @param leftKeyField the key field of the left source.
-     * @return the key field that should be used by the resultant joined stream.
-     */
-    static KeyField getOuterJoinedKeyField(


big-andy-coates · 2019-11-05T23:09:58Z

ksql-engine/src/main/java/io/confluent/ksql/planner/plan/KsqlStructuredDataOutputNode.java

-        getKeyField().legacy()
-    );
-
-    final SchemaKStream result = schemaKStream.withKeyField(resultKeyField);


withKeyField has been removed as its no longer needed. The only reason it was needed was to set the legacy (incorrect) keyField. With this PR this all goes away.

big-andy-coates · 2019-11-05T23:11:25Z

ksql-engine/src/main/java/io/confluent/ksql/structured/SchemaKStream.java

-      return doFindKeyColumn(selectExpressions, keyColumn).map(Column::ref);
-    }
-
-    private Optional<LegacyField> findLegacyKeyField(


this has been removed and everything else inlined into findKeyField

big-andy-coates · 2019-11-05T23:11:52Z

ksql-engine/src/test/java/io/confluent/ksql/codegen/CodeGenRunnerTest.java

@@ -119,7 +119,6 @@
    private static final int MAP_INDEX1 = 11;
    private static final int MAP_INDEX2 = 12;
    private static final int STRUCT_INDEX = 15;
-    private static final int DECIMAL_INDEX = 16;


removed as unused

big-andy-coates · 2019-11-05T23:12:09Z

ksql-engine/src/test/java/io/confluent/ksql/physical/PhysicalPlanBuilderTest.java

@@ -433,150 +433,6 @@ public void shouldHandleRightTableJoiningOnRowKey() {
    // Then: did not throw.
  }

-  @Test
-  public void shouldRepartitionLeftStreamIfNotCorrectKey_Legacy() {


all testing the legacy code path - so removed.

big-andy-coates · 2019-11-05T23:12:27Z

ksql-engine/src/test/java/io/confluent/ksql/planner/plan/DataSourceNodeTest.java

@@ -142,46 +127,29 @@
      Collections.emptyList()
  );

-  private final QueryId queryId = new QueryId("source-test");


these were all unused, so removed them

big-andy-coates · 2019-11-05T23:12:34Z

ksql-engine/src/test/java/io/confluent/ksql/planner/plan/DataSourceNodeTest.java

@@ -412,25 +378,6 @@ public void shouldBuildTableWithCorrectContext() {
        equalTo(ImmutableList.of("0", "reduce")));
  }

-
-  @SuppressWarnings("unchecked")
-  private DataSourceNode nodeWithMockTableSource() {


big-andy-coates · 2019-11-05T23:15:23Z

ksql-functional-tests/src/main/java/io/confluent/ksql/test/model/SourceNode.java

@@ -132,4 +136,32 @@ private static ConnectSchema makeTopLevelStructNoneOptional(final Schema schema)
    schema.fields().forEach(field -> builder.field(field.name(), field.schema()));
    return (ConnectSchema) builder.build();
  }
+
+  public static class Deserializer extends JsonDeserializer<SourceNode> {


the job of this deserializer is basically to distinguish between a source with no key field criteria specified, meaning the key field can be anything vs one where the key field is explicitly set to null, meaning the source should have no key field set.

I'm not aware of a better way to do this with Jackson...

big-andy-coates · 2019-11-05T23:16:19Z

ksql-functional-tests/src/test/java/io/confluent/ksql/test/ManualTopologyFileGeneratorTest.java

- * It is deliberately excluded from the test suite
- */
-@Ignore
-public final class ManualTopologyFileGeneratorTest {


I rolled this into TopologyFileGenerator ...

big-andy-coates · 2019-11-05T23:17:04Z

...0_6_0-pre/key-field_-_stream___initially_set___partition_by_(same)___key_in_value___aliasing

@@ -51,20 +50,11 @@ Topologies:
      --> KSTREAM-MAPVALUES-0000000002
      <-- KSTREAM-SOURCE-0000000000
    Processor: KSTREAM-MAPVALUES-0000000002 (stores: [])
-      --> KSTREAM-FILTER-0000000003
+      --> KSTREAM-MAPVALUES-0000000003


this topology has changed because the test was explicitly setting ksql.query.fields.key.legacy to true. With this setting now gone the unnecessary repartition is avoided.

big-andy-coates · 2019-11-05T23:18:03Z

ksql-functional-tests/src/test/resources/query-validation-tests/key-field.json

            "valueSchema": "STRUCT<FOO INT, KSQL_COL_1 BIGINT>"
          }
        ]
      }
    },
-    {
-      "name": "stream | initially null | group by (-) | key in value | no aliasing | legacy downstream query",


Removed these legacy downstream queries as the property they use no longer exists

rodesai

LGTM. Left one item inline.

ksql-engine/src/main/java/io/confluent/ksql/ddl/commands/DdlCommandExec.java

Follows on from confluentinc#3764 See comment: confluentinc#3764 (comment)

With legacy key fields now gone, (confluentinc#3764), we can now drop the final use of `Column.matches`.

With legacy key fields now gone, (#3764), we can now drop the final use of `Column.matches`.

Follows on from #3764 See comment: #3764 (comment)

* feat: drop legacy key field functionality BREAKING CHANGE This drops functionality that maintained a legacy version of the key field. This legacy version was often wrong and resulted in unnecessary repartition steps being performed. The functionality was previously required to ensure existing legacy queries continued to have the unnecessary repartitions to ensure no data loss. With a major version bump this can now be dropped.

big-andy-coates requested a review from a team as a code owner November 5, 2019 23:05

big-andy-coates commented Nov 5, 2019

View reviewed changes

chore: fix test

e340d70

big-andy-coates added a commit to big-andy-coates/ksql that referenced this pull request Nov 5, 2019

chore: update expected topologies post confluentinc#3764

7a765ce

big-andy-coates mentioned this pull request Nov 5, 2019

Update expected topologies post #3764 (MINOR) #3767

Merged

2 tasks

rodesai approved these changes Nov 6, 2019

View reviewed changes

ksql-engine/src/main/java/io/confluent/ksql/ddl/commands/DdlCommandExec.java Show resolved Hide resolved

big-andy-coates merged commit 5369dc2 into confluentinc:master Nov 6, 2019

big-andy-coates deleted the legacy_key_field branch November 6, 2019 08:44

big-andy-coates added a commit to big-andy-coates/ksql that referenced this pull request Nov 6, 2019

chore: drop legacy key field override from the ddl executor

565a3b5

Follows on from confluentinc#3764 See comment: confluentinc#3764 (comment)

big-andy-coates mentioned this pull request Nov 6, 2019

chore: drop legacy key field override from the ddl executor #3770

Merged

2 tasks

big-andy-coates added a commit to big-andy-coates/ksql that referenced this pull request Nov 6, 2019

refactor: remove final uses of fuzzy Column.matches

f7a54a4

With legacy key fields now gone, (confluentinc#3764), we can now drop the final use of `Column.matches`.

big-andy-coates mentioned this pull request Nov 6, 2019

refactor: remove final uses of fuzzy Column.matches #3771

Merged

2 tasks

big-andy-coates added a commit that referenced this pull request Nov 6, 2019

refactor: remove final uses of fuzzy Column.matches (#3771)

3a616a8

With legacy key fields now gone, (#3764), we can now drop the final use of `Column.matches`.

big-andy-coates added a commit that referenced this pull request Nov 11, 2019

chore: drop legacy key field override from the ddl executor (#3770)

4f20af6

Follows on from #3764 See comment: #3764 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: drop legacy key field functionality #3764

feat: drop legacy key field functionality #3764

big-andy-coates commented Nov 5, 2019 •

edited

Loading

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019 •

edited

Loading

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

big-andy-coates Nov 5, 2019

rodesai left a comment

feat: drop legacy key field functionality #3764

feat: drop legacy key field functionality #3764

Conversation

big-andy-coates commented Nov 5, 2019 • edited Loading

Description

Reviewing notes:

Testing done

Reviewer checklist

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 Nov 5, 2019 • edited Loading

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

rodesai left a comment

Choose a reason for hiding this comment

big-andy-coates commented Nov 5, 2019 •

edited

Loading

big-andy-coates Nov 5, 2019 •

edited

Loading