feat: Implement user defined delimiter for value format #3393

purplefox · 2019-09-21T07:49:07Z

Description

This PR implements configurable delimiters for value formats.

It is based on an original PR #2055 authored by @hasnat. Many thanks for your contribution!

Since the original PR was submitted the codebase has changed a lot so it was a complex merge, and I've had to reapply a lot of the changes as many of the originally modified classes do not exist or have moved in the current code base.

I've also added some more tests and implemented TAB and SPACE delimiters too.

Here’s a summary:

We introduce a new optional property VALUE_DELIMITER which can be used in WITH clauses when VALUE_FORMAT=‘DELIMITED’.
The default value for VALUE_DELIMITER is ','
VALUE_DELIMITER must be a single character or the special values ‘SPACE’ or ‘TAB’ which, unsurprisingly, represent space and tab delimited values.
We need to use special values for space and tab because
- With clause properties in KSQL are managed by subclasses of org.apache.kafka.common.config.AbstractConfig. This class trims (i.e. removes whitespace) from any values it maintains. Thus, it cannot be used where the value is just whitespace (e.g. a tab or space character).
- The KSQL cli currently does not support entering escaped tab characters, so it's much easier to use a different value to represent them.
Docs have been updated appropriately
Removed two param constructor of FormatInfo as it was only being used from tests
Updated DataGen

Testing done

Modified and added unit tests
Modified and added more QTT tests

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

update fork

* fork/master: (107 commits) copy-edited (confluentinc#2299) copy-edited (confluentinc#2299) remove unused map from command store `getRestoreCommands` (confluentinc#2296) Speed up integration tests. (confluentinc#2288) Move KSQL Clickstream demo to Examples repo (confluentinc#2270) Bump Confluent to 5.1.1-SNAPSHOT, Kafka to 2.1.1-SNAPSHOT Set Confluent to 5.1.0, Kafka to 2.1.0-cp1. Add ServiceContext (confluentinc#2243) Update to use CCL (confluentinc#2278) Remove redundant intro sentence in README.md (confluentinc#2277) Switch to use CCL (confluentinc#2275) Update readme for CCL (confluentinc#2276) Minor: add Hamcrest matchers for KsqlRestException and KsqlErrorMessage (confluentinc#2273) Update per relicensing (confluentinc#2274) CP-584: 5.1.0 changelog (confluentinc#2267) add null checks to min, max, and count aggregates; call Math.min/max for consistency (confluentinc#2246) Return and accept command topic offsets via REST API (confluentinc#2159) MINOR: Fix bug encountered when restoring RUN SCRIPT command. (again) (confluentinc#2265) More improvements to the way the CLI handles inline comments in multi-line statements. (confluentinc#2241) Fix issue where Avro subject does not exist (confluentinc#2260) ...

update master

…nt master

…d_format

ksql-parser/src/main/java/io/confluent/ksql/parser/properties/with/WithClauseProperties.java

rodesai

Looks good, couple of nits inline.

ksql-parser/src/main/java/io/confluent/ksql/parser/properties/with/WithClauseProperties.java

JimGalasyn

LGTM!

big-andy-coates

Thanks @purplefox

LGTM 'cept a load of nits and suggestions.

docs/developer-guide/syntax-reference.rst

ksql-common/src/main/java/io/confluent/ksql/properties/with/CommonCreateConfigs.java

ksql-serde/src/test/java/io/confluent/ksql/serde/delimited/KsqlDelimitedSerializerTest.java

ksql-functional-tests/src/test/resources/query-validation-tests/delimited-value-format.json

big-andy-coates

Thanks @purplefox

I think the code is looking much better with the introduction of the Delimiter class.

There are still a fair few comments outstanding from previous reviews, plus some new ones below.

ksql-common/src/main/java/io/confluent/ksql/configdef/ConfigValidators.java

ksql-common/src/main/java/io/confluent/ksql/properties/with/CommonCreateConfigs.java

ksql-common/src/main/java/io/confluent/ksql/serde/Delimiter.java

ksql-engine/src/main/java/io/confluent/ksql/analyzer/Analyzer.java

ksql-functional-tests/src/test/resources/query-validation-tests/delimited-value-format.json

...-parser/src/main/java/io/confluent/ksql/parser/properties/with/CreateSourceAsProperties.java

ksql-common/src/main/java/io/confluent/ksql/serde/Delimiter.java

big-andy-coates

LGTM 'cept for a few final nits.

Feel free to merge once you've cleaned them up.

big-andy-coates · 2019-09-25T18:33:30Z

ksql-common/src/main/java/io/confluent/ksql/configdef/ConfigValidators.java

+   * @param parser the parser.
+   * @return the validator
+   */
+  public static Validator parses(final Function<String, ?> parser) {


Would benefit from a unit test - there's some in the patch I sent you.

big-andy-coates · 2019-09-25T19:29:34Z

ksql-common/src/main/java/io/confluent/ksql/serde/Delimiter.java

+
+  private final char delimiter;
+
+  public static Delimiter parse(final char ch) {


lol - I only mean change the of(String) to parse. I think this one with a single char should of remained of. sorry. Should have been more explicit!

big-andy-coates · 2019-09-25T19:32:11Z

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

+        "CREATE STREAM S2 as SELECT id, name, value FROM test;"
+      ],
+      "inputs": [
+        {"topic": "test_topic", "key": 0, "value": "0,zero,0", "timestamp": 0},


BTW, for future reference, (no need to change), where the test case doesn't really care about the key or timestamp they can be excluded to make the test more succinct, e.g.

Suggested change

{"topic": "test_topic", "key": 0, "value": "0,zero,0", "timestamp": 0},

{"topic": "test_topic", "value": "0,zero,0"},

big-andy-coates · 2019-09-25T19:58:50Z

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

+      "statements": [
+        "CREATE STREAM TEST WITH (kafka_topic='test_topic', value_format='JSON', value_delimiter='|');"
+      ],
+      "topics": [


As mentioned before - if you correct the error handling to store the properties class being created in an invalid state, (which it looks like you have), you shouldn't need this topics second.

...-parser/src/main/java/io/confluent/ksql/parser/properties/with/CreateSourceAsProperties.java

big-andy-coates · 2019-09-25T20:00:39Z

ksql-serde/src/main/java/io/confluent/ksql/serde/delimited/KsqlDelimitedSerdeFactory.java

+
+  public KsqlDelimitedSerdeFactory(final Optional<Delimiter> delimiter) {
+    this.csvFormat =
+        CSVFormat.DEFAULT.withDelimiter(delimiter.orElse(DEFAULT_DELIMITER).getDelimiter());


nit: strange formatting... blank line etc.

I'd probably go with:

this.csvFormat = CSVFormat.DEFAULT .withDelimiter(delimiter.orElse(DEFAULT_DELIMITER).getDelimiter());

It it were me.

…d_format

hasnat and others added 30 commits October 31, 2018 18:41

Merge pull request #1 from confluentinc/master

51f416f

update fork

Added optional for FORMAT_VALUE DELIMITED

7945d62

Tests + Code review changes

6710357

Fix formatting in sytax reference docs

e3481be

Adds option to use custom delimiters when VALUE_FORMAT='DELIMITED'

eb230a4

Merge branch 'master' into add_optional_delimiter_for_delimited_format

4148ba9

Merge branch 'master' into add_optional_delimiter_for_delimited_format

007ad4a

Allow only single character delimiters

c6d0092

Merge branch 'master' into add_optional_delimiter_for_delimited_format

426b8f6

Updates to master merge

c774b09

Adds seralizer with custom delimiter

aaf6fa4

Merge branch 'master' into add_optional_delimiter_for_delimited_format

5dbcfaa

Fix max line issue

077699d

Fix logger classes

ab21e53

Try default delimiter type with custom character

7794be0

Use utf 8

ef4ca56

fix query

2b2491a

Fix value delimiter WITH argument

d1a64e3

use default with delimiter

5c58d25

remove old test

8eecc8d

Merge pull request #2 from confluentinc/master

a4bed15

update master

Merge origin/master

c29017d

style fix

66ab934

Missing default

3dce9e9

bug fixes

9676843

more style fixes

a5b496a

fix: Merged PR into master

5f27f02

fix: Get the PR to work as the original author intended against curre…

b1075d4

…nt master

Merge branch 'master' into hasnat-add_optional_delimiter_for_delimite…

924aa36

…d_format

rodesai reviewed Sep 22, 2019

View reviewed changes

ksql-parser/src/main/java/io/confluent/ksql/parser/properties/with/WithClauseProperties.java Outdated Show resolved Hide resolved

rodesai approved these changes Sep 22, 2019

View reviewed changes

ksql-parser/src/main/java/io/confluent/ksql/parser/properties/with/WithClauseProperties.java Outdated Show resolved Hide resolved

JimGalasyn approved these changes Sep 23, 2019

View reviewed changes

fix:nit

543fdee

big-andy-coates reviewed Sep 24, 2019

View reviewed changes

Tim Fox added 9 commits September 24, 2019 16:23

fix:nits

d2699ef

fix:nits

49426b8

fix:nits

53cdf5f

Improved CommonCreateConfigs description for value_delimiter

1e421ec

Use validator to check value_delimiter is not empty

2f05ad3

Use concatenated delimiter named values in error message

8cf99d3

Use value delimiter from source if sink doesn't specify one

1d7a223

Remove file that shouldn't be there

33e7358

Created delimiter class

0bbbb37

big-andy-coates reviewed Sep 25, 2019

View reviewed changes

ksql-functional-tests/src/test/resources/query-validation-tests/delimited-value-format.json Outdated Show resolved Hide resolved

ksql-functional-tests/src/test/resources/query-validation-tests/delimited-value-format.json Outdated Show resolved Hide resolved

big-andy-coates reviewed Sep 25, 2019

View reviewed changes

Review comments

854b6ec

purplefox requested a review from big-andy-coates September 25, 2019 17:08

Tim Fox added 3 commits September 25, 2019 19:10

Added getter for delimiter

4e7b0a7

more nits

ea33711

Add invalid value to exception message for Delimiter

fa12f03

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

View reviewed changes

Tim Fox added 5 commits September 25, 2019 22:19

review

a261c35

Merge branch 'master' into hasnat-add_optional_delimiter_for_delimite…

6f322e5

…d_format

fixed test

0552373

fixed get of delimiter

6c27d26

Fixed merge conflict

87f8342

purplefox merged commit b84d0aa into confluentinc:master Sep 26, 2019

purplefox deleted the hasnat-add_optional_delimiter_for_delimited_format branch September 26, 2019 07:27

big-andy-coates mentioned this pull request Feb 7, 2020

DataGen value_delimiter option not wired up. #4481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement user defined delimiter for value format #3393

feat: Implement user defined delimiter for value format #3393

purplefox commented Sep 21, 2019 •

edited

Loading

rodesai left a comment

JimGalasyn left a comment

big-andy-coates left a comment

big-andy-coates left a comment

big-andy-coates left a comment

big-andy-coates Sep 25, 2019

big-andy-coates Sep 25, 2019

big-andy-coates Sep 25, 2019

big-andy-coates Sep 25, 2019

big-andy-coates Sep 25, 2019


		private final char delimiter;

		public static Delimiter parse(final char ch) {

	{"topic": "test_topic", "key": 0, "value": "0,zero,0", "timestamp": 0},
	{"topic": "test_topic", "value": "0,zero,0"},

feat: Implement user defined delimiter for value format #3393

feat: Implement user defined delimiter for value format #3393

Conversation

purplefox commented Sep 21, 2019 • edited Loading

Description

Testing done

Reviewer checklist

rodesai left a comment

Choose a reason for hiding this comment

JimGalasyn left a comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates Sep 25, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 25, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 25, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 25, 2019

Choose a reason for hiding this comment

big-andy-coates Sep 25, 2019

Choose a reason for hiding this comment

purplefox commented Sep 21, 2019 •

edited

Loading