Add support for tuple ClickHouse #29715

mzitnik · 2023-12-12T08:02:37Z

Add support for tuple ClickHouse

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

github-actions · 2023-12-12T09:34:40Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @robertwb for label java.
R: @chamikaramj for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

mshustov · 2023-12-12T18:00:53Z

Cc @Abacn since you've reviewed the last 2 PRs adding ClickHouse data types

Abacn

Thanks, did a quick pass and let some comments. However I am going to travel for the next weeks. Would be great if could find another reviewer or until late December

Abacn · 2023-12-12T18:47:26Z

CHANGES.md

@@ -68,6 +68,7 @@
 * Adding support for LowCardinality DataType in ClickHouse (Java) ([#29533](https://github.com/apache/beam/pull/29533)).
 * Added support for handling bad records to KafkaIO (Java) ([#29546](https://github.com/apache/beam/pull/29546))
 * Add support for generating text embeddings in MLTransform for Vertex AI and Hugging Face Hub models.([#29564](https://github.com/apache/beam/pull/29564))
+* Adding support for Tuples DataType in ClickHouse (Java) ([Tuple Support](https://github.com/apache/beam/pull/29715)).


We can combine this with LowCardinality DataType above, e.g.

Adding support for LowCardinality and Tuples DataType in ClickHouse (Java) ([#29533](https://github.com/apache/beam/pull/29533), [#29715](https://github.com/apache/beam/pull/29715)).

Release cut is today so this may not get into 2.53.0. Could leave it as is and fix when the PR is finalized

Abacn · 2023-12-12T18:50:44Z

sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java

@@ -112,7 +113,8 @@
 * </table>
 *
 * Nullable row columns are supported through Nullable type in ClickHouse. Low cardinality hint is
- * supported through LowCardinality DataType in ClickHouse.
+ * supported through LowCardinality DataType in ClickHouse supported through Tuple DataType in


Consider put the documentation Tuple to the table above. Broken sentence here currently.

Please check this sentence

Abacn · 2023-12-13T17:49:58Z

sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java

+            .map(s -> s.trim().replaceAll(" +", "':;"))
+            .collect(Collectors.toList());
+    String content =
+        String.join(",", l).trim().replaceAll("Tuple\\(", "Tuple('").replaceAll(",", ",'");


The caller has a condition if (type.toLowerCase().trim().startsWith("tuple(")) { below, so here payload always (case insensitive) starts with "tuple(", so here the replace of "Tuple\(" won't work

The condition is on with lower case, but the actual parsing is on the original string

I mean below when " if (type.toLowerCase().trim().startsWith("tuple(")) {" the this preprocessing will be executed.

So Tuple( (with backslash) is considered here but not the condition in the caller there. Coild this cause issue?

You're right. Would you mind adding some comments about the proprocessing rule and example, for example, sth like // Tuple(a String, b Integer) -> ...

Abacn · 2023-12-13T17:50:55Z

sdks/java/io/clickhouse/src/main/javacc/ColumnTypeParser.jj

@@ -99,6 +103,9 @@ TOKEN :
  | < EQ             : "=" >
  | < BOOL           : "BOOL" >
  | < LOWCARDINALITY : "LOWCARDINALITY" >
+  | < TUPLE          : "TUPLE" >
+  | < COLON          : ":" >


Is COLON and SEMI_COLON for tuple or another feature?

Using that since i have some ambiguity in parsing (looking for other options), but for now decided to leave it that way

If it's get added to SDK then there is expectation of backward compatibility. So I would prefer to keep consistent with ClickHouse's SQL syntax

It's a temporary solution since there is some ambiguity when using javacc having difficulty to detecting between tokens and strings

Removed COLON and SEMI_ COLON

Abacn · 2023-12-13T17:52:39Z

sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/TableSchemaTest.java

+    Map<String, ColumnType> m2 = new HashMap<>();
+    m2.put("a1", ColumnType.STRING);
+    m2.put("b", ColumnType.BOOL);
+    ColumnType columnType02 = ColumnType.parse("Tuple('a1':;String,'b':;Bool)");


Is :; a standard syntax for ClickHouse? https://clickhouse.com/docs/en/sql-reference/data-types/tuple seems not mentioning this pattern.

@Abacn Removed :;

github-actions · 2023-12-21T12:13:50Z

Reminder, please take a look at this pr: @robertwb @chamikaramj

github-actions · 2023-12-26T12:13:40Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @kennknowles for label java.
R: @johnjcasey for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

Abacn

Thanks, the syntax now looks good and consistent with clickhouse documentation. Replied to previous comment

Abacn · 2024-01-02T01:00:26Z

CHANGES.md

 * Added support for handling bad records to KafkaIO (Java) ([#29546](https://github.com/apache/beam/pull/29546))
 * Add support for generating text embeddings in MLTransform for Vertex AI and Hugging Face Hub models.([#29564](https://github.com/apache/beam/pull/29564))
 * NATS IO connector added (Go) ([#29000](https://github.com/apache/beam/issues/29000)).
+* Adding support for LowCardinality and Tuples DataType in ClickHouse (Java) ([#29533](https://github.com/apache/beam/pull/29533), [#29715](https://github.com/apache/beam/pull/29715)).


LowCardinality will go to 2.53.0 but Tuples does not, as 2.53.0 is already cut

Abacn · 2024-01-02T01:01:05Z

sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java

@@ -112,7 +113,8 @@
 * </table>
 *
 * Nullable row columns are supported through Nullable type in ClickHouse. Low cardinality hint is
- * supported through LowCardinality DataType in ClickHouse.
+ * supported through LowCardinality DataType in ClickHouse supported through Tuple DataType in


Please check this sentence

Abacn · 2024-01-02T01:10:58Z

sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java

+            .map(s -> s.trim().replaceAll(" +", "':;"))
+            .collect(Collectors.toList());
+    String content =
+        String.join(",", l).trim().replaceAll("Tuple\\(", "Tuple('").replaceAll(",", ",'");


I mean below when " if (type.toLowerCase().trim().startsWith("tuple(")) {" the this preprocessing will be executed.

So Tuple( (with backslash) is considered here but not the condition in the caller there. Coild this cause issue?

Abacn · 2024-01-02T01:18:15Z

waiting on author

mzitnik · 2024-01-02T07:26:26Z

@Abacn
Regarding your last comment, "Tuple\(" replaceAll method expects regex since we are looking for "Tuple\(" it's invalid to place ( without \ in regex and the double \ is because we can not represent single \ in quotes. Correct me if I am wrong here.

Abacn · 2024-01-02T15:13:58Z

@Abacn Regarding your last comment, "Tuple(" replaceAll method expects regex since we are looking for "Tuple(" it's invalid to place ( without \ in regex and the double \ is because we can not represent single \ in quotes. Correct me if I am wrong here.

You're right, sorry for confusion

Abacn

Thanks!

* First version WIP * Implement write tuple in RowBinary format * Added complex tuple test * Disable debug of javacc * Move tuple preprocessing logic * Adding to CHANGES.md & auto generated docs. * Fix CHANGES.md & fix docs * Fix spotless syntax * Remove :; from parsing. Only adding ' to field name. * Fix CHANGES.md to the correct version * Change new types javadoc --------- Co-authored-by: mzitnik <mark.zitnik@clickhouse.com>

erube · 2024-02-13T16:56:58Z

sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java

+      case TUPLE:
+        List<Schema.Field> fields =
+            columnType.tupleTypes().entrySet().stream()
+                .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))


Hi @mzitnik,
Shouldn't each key inside a tuple be mapped to their respective data type ? Any reason for being DATETIME?

Hi,

This quick adaptation seems to work:

case TUPLE: List<TableSchema.Column> columns = columnType.tupleTypes().entrySet().stream().map( x -> TableSchema.Column.of(x.getKey(), x.getValue()) ).collect(Collectors.toList()); TableSchema tupleSchema = TableSchema.of(columns.toArray(new TableSchema.Column[0])); Schema tupleAsRowSchema = getEquivalentSchema(tupleSchema); return Schema.FieldType.row(tupleAsRowSchema);

Hi,

Also, it seems that Array(Tuple(*)) is not supported when calling ClickHouseIO.getTableSchema.

This seems to fix the issue :
if (type.toLowerCase().trim().startsWith("tuple(")) { String content = tuplePreprocessing(type); columnType = TableSchema.ColumnType.parse(content); } else if (type.toLowerCase().trim().startsWith("array(tuple(")) { String content = tuplePreprocessing(type); columnType = TableSchema.ColumnType.parse(content); }

@erube and @wattache, thanks for the feedback i will fork and also add some tests to cover Array(Tuple(*))

mzitnik added 5 commits December 11, 2023 21:54

First version WIP

3bbbe2f

Implement write tuple in RowBinary format

a3a1850

Added complex tuple test

53e4e81

Disable debug of javacc

a1d5182

Move tuple preprocessing logic

a3cafa6

github-actions bot added java io clickhouse labels Dec 12, 2023

mzitnik marked this pull request as draft December 12, 2023 08:04

Adding to CHANGES.md & auto generated docs.

120bb2b

mzitnik marked this pull request as ready for review December 12, 2023 09:10

github-actions bot added the Next Action: Reviewers label Dec 12, 2023

Merge branch 'master' into add-support-for-tuple-clickhouse

256187f

Merge branch 'master' into add-support-for-tuple-clickhouse

c1b3f98

Abacn reviewed Dec 13, 2023

View reviewed changes

mzitnik added 3 commits December 13, 2023 20:08

Fix CHANGES.md & fix docs

a477cbc

Fix spotless syntax

ccfdc37

Remove :; from parsing. Only adding ' to field name.

67434ef

github-actions bot added the slow-review label Dec 21, 2023

Merge branch 'apache:master' into add-support-for-tuple-clickhouse

f27b943

github-actions bot removed the slow-review label Dec 26, 2023

Abacn reviewed Jan 2, 2024

View reviewed changes

github-actions bot added Next Action: Author and removed Next Action: Reviewers labels Jan 2, 2024

Merge branch 'apache:master' into add-support-for-tuple-clickhouse

b4ce111

github-actions bot added Next Action: Reviewers and removed Next Action: Author labels Jan 2, 2024

mzitnik added 2 commits January 2, 2024 09:04

Fix CHANGES.md to the correct version

748dca6

Change new types javadoc

57cb6a5

Abacn approved these changes Jan 2, 2024

View reviewed changes

Abacn merged commit 1c1cfa8 into apache:master Jan 2, 2024
20 checks passed

erube reviewed Feb 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tuple ClickHouse #29715

Add support for tuple ClickHouse #29715

mzitnik commented Dec 12, 2023 •

edited

github-actions bot commented Dec 12, 2023

mshustov commented Dec 12, 2023

Abacn left a comment

Abacn Dec 12, 2023

Abacn Dec 13, 2023

Abacn Dec 12, 2023

Abacn Jan 2, 2024

Abacn Dec 13, 2023

mzitnik Dec 13, 2023

Abacn Jan 2, 2024

Abacn Jan 2, 2024

Abacn Dec 13, 2023

mzitnik Dec 13, 2023

Abacn Dec 13, 2023

mzitnik Dec 13, 2023

mzitnik Dec 13, 2023

Abacn Dec 13, 2023

mzitnik Dec 13, 2023 •

edited

github-actions bot commented Dec 21, 2023

github-actions bot commented Dec 26, 2023

Abacn left a comment

Abacn Jan 2, 2024

Abacn Jan 2, 2024

Abacn Jan 2, 2024

Abacn commented Jan 2, 2024

mzitnik commented Jan 2, 2024

Abacn commented Jan 2, 2024

Abacn left a comment

erube Feb 13, 2024

wattache Feb 13, 2024 •

edited

wattache Feb 14, 2024

mzitnik Feb 28, 2024 •

edited

Add support for tuple ClickHouse #29715

Add support for tuple ClickHouse #29715

Conversation

mzitnik commented Dec 12, 2023 • edited