feat: use coherent naming scheme for generated java code #3417

agavra · 2019-09-25T22:09:01Z

Description

This patch ensures that all generated code uses valid Java identifiers instead of arbitrary column names by generating varX where X is the column index for the parameter being passed in. Note that the only "real" changes in this PR are in SqlToJavaVisitor and CodeGenRunner (both of which are single line changes).

As part of this change I also:

Column also has an index
LogicalSchema.Builder tracks Column Indices
LogicalSchema uses the builder to do internal copies to maintain unique indices (when copying metadata/keys into value)

Testing done

mvn clean install

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

vcrfxia

Thanks @agavra -- LGTM with a question inline.

vcrfxia · 2019-09-26T22:15:50Z

ksql-common/src/main/java/io/confluent/ksql/schema/ksql/LogicalSchema.java

      final List<Column> suppliedKey = keyBuilder.build();

      final List<Column> key = suppliedKey.isEmpty()
          ? IMPLICIT_KEY_SCHEMA
          : suppliedKey;

      return new LogicalSchema(
-          METADATA_SCHEMA,
+          metadata.isEmpty() ? METADATA_SCHEMA : metadata,


Not entirely understanding this change. Is the idea that external Builder instances will continue to use the default metadata schema, but that internal Builder instances will copy the metadata around?

The metadata schema is not updated anywhere, is it? So this is equivalent to always returning the default metadata schema, except "safer" in that we copy when we should. Is that correct?

I thought the comment would help here - basically the metadata is actually updated when we add/remove aliases :( I'll be trying to remove that in the long run.

// used only for internal copy (keeps the alias from the original schema)

Ah got'cha. And all of that is still managed by this class, which is why the metadataColumns() method can be private. Thanks for clarifying!

purplefox · 2019-09-27T08:07:13Z

Hey @agavra , could you give an example of some generated code that's changed because of these changes?

big-andy-coates

Thanks @agavra

I've got no issue with the core change to the code generation - this is a great improvement IMHO.

However, I do have an objection to the change to Column. It smells! See comment inline...

big-andy-coates · 2019-09-27T12:43:07Z

ksql-common/src/main/java/io/confluent/ksql/schema/ksql/Column.java

+   * Unknown index, used for key columns that have not
+   * been resolved to a value column.
+   */
+  public static final int UNKNOWN_IDX = -1;


why not just have OptionalInt index, rather than some magic special number?

big-andy-coates · 2019-09-27T12:58:21Z

ksql-common/src/main/java/io/confluent/ksql/schema/ksql/Column.java

  private final ColumnRef ref;
  private final SqlType type;
+  private final int index;


Hummm.... index is not included in equals and hashCode.

I'm guessing you're doing this as otherwise lots of tests and other things start to fail, right?

These feels wrong and will likely cause bugs or at least tests that don't test what people think they do.

It looks like the functionality in this PR can be implemented without the change to this class.

I suggest one of the following approaches:

Don't make this change to this class, and just continue to use the existing valueColumnIndex impl in LogicalSchema for now.

Make this change, but implement hashCode and equals properly. As part of this we should avoid building instances of Column to pass to things where we don't care about the index. Escentially, if a method only cares about the name and type, then that's not a Column with this change. e.g. the following is an example of existing code that would become a misuse of Column. The doFindKeyColumn method should take the name and type as separate params:

return doFindKeyColumn(selectExpressions, Column.of(keyField.name(), keyField.type())) .map(field -> LegacyField.of(ColumnName.of(field.fullName()), field.type()));

Leave column as-is, but change LogicalSchema to track the indexes better.

Build the field -> index mapping just in the java generation code.

Something else.

TBH, there's a lot of changes going in around the logical schema, columns, and copying of rowkey and rowtime into the value schema. Once the dust settles on all this good refactoring it might well be that the only place that cares about indexes is the java code builder. Hence, I'd urge you to just go with option 1 above for now.

big-andy-coates · 2019-09-27T13:00:03Z

ksql-common/src/test/java/io/confluent/ksql/schema/ksql/ColumnTest.java

@@ -51,7 +51,8 @@ public void shouldImplementEqualsProperly() {
    new EqualsTester()
        .addEqualityGroup(
            Column.of(SOME_NAME, SqlTypes.INTEGER),
-            Column.of(SOME_NAME, SqlTypes.INTEGER)
+            Column.of(SOME_NAME, SqlTypes.INTEGER),
+            Column.of(SOME_NAME, SqlTypes.INTEGER).withIndex(1) // index is not considered


big-andy-coates · 2019-09-27T13:00:19Z

ksql-execution/src/main/java/io/confluent/ksql/execution/codegen/CodeGenRunner.java

@@ -180,7 +180,7 @@ private void addParameter(final Column schemaColumn) {
      parameters.add(new ParameterType(
          SQL_TO_JAVA_TYPE_CONVERTER.toJavaType(schemaColumn.type()),
          schemaColumn.fullName(),
-          schemaColumn.fullName().replace(".", "_"),
+          CodeGenUtils.paramName(schemaColumn),


big-andy-coates · 2019-09-27T13:02:31Z

ksql-execution/src/main/java/io/confluent/ksql/execution/codegen/CodeGenUtils.java

+  }
+
+  static String paramName(final Column column) {
+    KsqlPreconditions.checkArgument(


This throws an KsqlException which is reserved for user errors. This is not a user error...

big-andy-coates · 2019-09-27T13:17:53Z

Hey @agavra , could you give an example of some generated code that's changed because of these changes?

@purplefox the sorts of changes can be see in the SqlToJavaVisitorTest.java. However, in short, KSQL works by generating text containing java code and the compiling it. SqlToJavaVisitorTest is the class that builds the java code. Previously, it would use the column names in the java code. This meant our column names needed to be valid Java identifiers. With @almog's change we replace the use of column names with indexed variables, so rather than myFirstColumn you get var0 in the Java code.

@big-andy-coates

making major change after @big-andy-coates' suggestion

big-andy-coates

Now that's a lot smaller and more contained!

Thanks @agavra, LGTM

agavra requested a review from a team as a code owner September 25, 2019 22:09

agavra mentioned this pull request Sep 26, 2019

feat(static): allow logical schema to have fields in any order #3422

Merged

2 tasks

vcrfxia previously approved these changes Sep 26, 2019

View reviewed changes

vcrfxia requested a review from a team September 26, 2019 22:16

big-andy-coates suggested changes Sep 27, 2019

View reviewed changes

feat: use coherent naming scheme for generated java code

57fdecf

agavra force-pushed the java_identifiers branch from 8371eb8 to 57fdecf Compare September 27, 2019 17:37

agavra requested review from big-andy-coates, vcrfxia and a team September 27, 2019 17:38

big-andy-coates approved these changes Sep 27, 2019

View reviewed changes

agavra merged commit 06a2a57 into confluentinc:master Sep 28, 2019

agavra deleted the java_identifiers branch September 28, 2019 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use coherent naming scheme for generated java code #3417

feat: use coherent naming scheme for generated java code #3417

agavra commented Sep 25, 2019

vcrfxia left a comment

vcrfxia Sep 26, 2019

agavra Sep 26, 2019 •

edited

Loading

vcrfxia Sep 26, 2019

purplefox commented Sep 27, 2019

big-andy-coates left a comment

big-andy-coates Sep 27, 2019

big-andy-coates Sep 27, 2019

big-andy-coates Sep 27, 2019

big-andy-coates Sep 27, 2019

big-andy-coates Sep 27, 2019

big-andy-coates commented Sep 27, 2019

big-andy-coates left a comment

feat: use coherent naming scheme for generated java code #3417

feat: use coherent naming scheme for generated java code #3417

Conversation

agavra commented Sep 25, 2019

Description

Testing done

Reviewer checklist

vcrfxia left a comment

Choose a reason for hiding this comment

vcrfxia Sep 26, 2019

Choose a reason for hiding this comment

agavra Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

vcrfxia Sep 26, 2019

Choose a reason for hiding this comment

purplefox commented Sep 27, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates Sep 27, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 27, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 27, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 27, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 27, 2019

Choose a reason for hiding this comment

big-andy-coates commented Sep 27, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

agavra Sep 26, 2019 •

edited

Loading