You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "mzitnik (via GitHub)" <gi...@apache.org> on 2023/12/12 08:02:39 UTC
[PR] Add support for tuple ClickHouse [beam]
mzitnik opened a new pull request, #29715:
URL: https://github.com/apache/beam/pull/29715
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](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "wattache (via GitHub)" <gi...@apache.org>.
wattache commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1488260740
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -111,6 +112,14 @@ public static Schema.FieldType getEquivalentFieldType(ColumnType columnType) {
return Schema.FieldType.STRING;
case BOOL:
return Schema.FieldType.BOOLEAN;
+ case TUPLE:
+ List<Schema.Field> fields =
+ columnType.tupleTypes().entrySet().stream()
+ .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))
Review Comment:
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29715:
URL: https://github.com/apache/beam/pull/29715#issuecomment-1866147622
Reminder, please take a look at this pr: @robertwb @chamikaramj
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #29715:
URL: https://github.com/apache/beam/pull/29715
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1425714474
##########
sdks/java/io/clickhouse/src/main/javacc/ColumnTypeParser.jj:
##########
@@ -99,6 +103,9 @@ TOKEN :
| < EQ : "=" >
| < BOOL : "BOOL" >
| < LOWCARDINALITY : "LOWCARDINALITY" >
+ | < TUPLE : "TUPLE" >
+ | < COLON : ":" >
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mshustov (via GitHub)" <gi...@apache.org>.
mshustov commented on PR #29715:
URL: https://github.com/apache/beam/pull/29715#issuecomment-1852547079
Cc @Abacn since you've reviewed the last 2 PRs adding ClickHouse data types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "wattache (via GitHub)" <gi...@apache.org>.
wattache commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1489201711
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -111,6 +112,14 @@ public static Schema.FieldType getEquivalentFieldType(ColumnType columnType) {
return Schema.FieldType.STRING;
case BOOL:
return Schema.FieldType.BOOLEAN;
+ case TUPLE:
+ List<Schema.Field> fields =
+ columnType.tupleTypes().entrySet().stream()
+ .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))
Review Comment:
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);
}
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1425704569
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java:
##########
@@ -475,6 +477,15 @@ abstract static class Builder<T> {
}
}
+ private static String tuplePreprocessing(String payload) {
+ List<String> l =
+ Arrays.stream(payload.trim().split(","))
+ .map(s -> s.trim().replaceAll(" +", "':;"))
+ .collect(Collectors.toList());
+ String content =
+ String.join(",", l).trim().replaceAll("Tuple\\(", "Tuple('").replaceAll(",", ",'");
Review Comment:
The condition is on with lower case, but the actual parsing is on the original string
##########
sdks/java/io/clickhouse/src/main/javacc/ColumnTypeParser.jj:
##########
@@ -99,6 +103,9 @@ TOKEN :
| < EQ : "=" >
| < BOOL : "BOOL" >
| < LOWCARDINALITY : "LOWCARDINALITY" >
+ | < TUPLE : "TUPLE" >
+ | < COLON : ":" >
Review Comment:
Using that since i have some ambiguity in parsing (looking for other options), but for now decided to leave it that way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #29715:
URL: https://github.com/apache/beam/pull/29715#issuecomment-1874153780
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29715:
URL: https://github.com/apache/beam/pull/29715#issuecomment-1851644740
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1425701974
##########
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)).
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #29715:
URL: https://github.com/apache/beam/pull/29715#issuecomment-1873551200
waiting on author
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1506273978
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -111,6 +112,14 @@ public static Schema.FieldType getEquivalentFieldType(ColumnType columnType) {
return Schema.FieldType.STRING;
case BOOL:
return Schema.FieldType.BOOLEAN;
+ case TUPLE:
+ List<Schema.Field> fields =
+ columnType.tupleTypes().entrySet().stream()
+ .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))
Review Comment:
@erube and @wattache, thanks for the feedback i will fork and also add some tests to cover Array(Tuple(*))
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -111,6 +112,14 @@ public static Schema.FieldType getEquivalentFieldType(ColumnType columnType) {
return Schema.FieldType.STRING;
case BOOL:
return Schema.FieldType.BOOLEAN;
+ case TUPLE:
+ List<Schema.Field> fields =
+ columnType.tupleTypes().entrySet().stream()
+ .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))
Review Comment:
@erube and @wattache, thanks for the feedback i will fork and also add some tests to cover `Array(Tuple(*))`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on PR #29715:
URL: https://github.com/apache/beam/pull/29715#issuecomment-1873688514
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1425795113
##########
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/TableSchemaTest.java:
##########
@@ -196,4 +197,55 @@ public void testEquivalentSchema() {
assertEquals(expected, TableSchema.getEquivalentSchema(tableSchema));
}
+
+ @Test
+ public void testParseTupleSingle() {
+ Map<String, ColumnType> m1 = new HashMap<>();
+ m1.put("s", ColumnType.STRING);
+ ColumnType columnType01 = ColumnType.parse("Tuple('s':;String)");
+ assertEquals(ColumnType.tuple(m1), columnType01);
+ }
+
+ @Test
+ public void testParseTupleDouble() {
+ 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)");
Review Comment:
Removed `:;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1425723246
##########
sdks/java/io/clickhouse/src/main/javacc/ColumnTypeParser.jj:
##########
@@ -99,6 +103,9 @@ TOKEN :
| < EQ : "=" >
| < BOOL : "BOOL" >
| < LOWCARDINALITY : "LOWCARDINALITY" >
+ | < TUPLE : "TUPLE" >
+ | < COLON : ":" >
Review Comment:
It's a temporary solution since there is some ambiguity when using `javacc` having difficulty to detecting between tokens and strings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1425796068
##########
sdks/java/io/clickhouse/src/main/javacc/ColumnTypeParser.jj:
##########
@@ -99,6 +103,9 @@ TOKEN :
| < EQ : "=" >
| < BOOL : "BOOL" >
| < LOWCARDINALITY : "LOWCARDINALITY" >
+ | < TUPLE : "TUPLE" >
+ | < COLON : ":" >
Review Comment:
Removed `COLON` and `SEMI_ COLON `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1425795113
##########
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/TableSchemaTest.java:
##########
@@ -196,4 +197,55 @@ public void testEquivalentSchema() {
assertEquals(expected, TableSchema.getEquivalentSchema(tableSchema));
}
+
+ @Test
+ public void testParseTupleSingle() {
+ Map<String, ColumnType> m1 = new HashMap<>();
+ m1.put("s", ColumnType.STRING);
+ ColumnType columnType01 = ColumnType.parse("Tuple('s':;String)");
+ assertEquals(ColumnType.tuple(m1), columnType01);
+ }
+
+ @Test
+ public void testParseTupleDouble() {
+ 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)");
Review Comment:
@Abacn Removed `:;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1439546775
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java:
##########
@@ -475,6 +477,15 @@ abstract static class Builder<T> {
}
}
+ private static String tuplePreprocessing(String payload) {
+ List<String> l =
+ Arrays.stream(payload.trim().split(","))
+ .map(s -> s.trim().replaceAll(" +", "':;"))
+ .collect(Collectors.toList());
+ String content =
+ String.join(",", l).trim().replaceAll("Tuple\\(", "Tuple('").replaceAll(",", ",'");
Review Comment:
You're right. Would you mind adding some comments about the proprocessing rule and example, for example, sth like `// Tuple(a String, b Integer) -> ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1424446081
##########
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
Review Comment:
Consider put the documentation Tuple to the table above. Broken sentence here currently.
##########
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)).
Review Comment:
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)).
```
##########
sdks/java/io/clickhouse/src/test/java/org/apache/beam/sdk/io/clickhouse/TableSchemaTest.java:
##########
@@ -196,4 +197,55 @@ public void testEquivalentSchema() {
assertEquals(expected, TableSchema.getEquivalentSchema(tableSchema));
}
+
+ @Test
+ public void testParseTupleSingle() {
+ Map<String, ColumnType> m1 = new HashMap<>();
+ m1.put("s", ColumnType.STRING);
+ ColumnType columnType01 = ColumnType.parse("Tuple('s':;String)");
+ assertEquals(ColumnType.tuple(m1), columnType01);
+ }
+
+ @Test
+ public void testParseTupleDouble() {
+ 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)");
Review Comment:
Is `:;` a standard syntax for ClickHouse? https://clickhouse.com/docs/en/sql-reference/data-types/tuple seems not mentioning this pattern.
##########
sdks/java/io/clickhouse/src/main/javacc/ColumnTypeParser.jj:
##########
@@ -99,6 +103,9 @@ TOKEN :
| < EQ : "=" >
| < BOOL : "BOOL" >
| < LOWCARDINALITY : "LOWCARDINALITY" >
+ | < TUPLE : "TUPLE" >
+ | < COLON : ":" >
Review Comment:
Is COLON and SEMI_COLON for tuple or another feature?
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java:
##########
@@ -475,6 +477,15 @@ abstract static class Builder<T> {
}
}
+ private static String tuplePreprocessing(String payload) {
+ List<String> l =
+ Arrays.stream(payload.trim().split(","))
+ .map(s -> s.trim().replaceAll(" +", "':;"))
+ .collect(Collectors.toList());
+ String content =
+ String.join(",", l).trim().replaceAll("Tuple\\(", "Tuple('").replaceAll(",", ",'");
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29715:
URL: https://github.com/apache/beam/pull/29715#issuecomment-1869499014
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1439110561
##########
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
Review Comment:
Please check this sentence
##########
CHANGES.md:
##########
@@ -98,10 +98,10 @@
* TextIO now supports skipping multiple header lines (Java) ([#17990](https://github.com/apache/beam/issues/17990)).
* Python GCSIO is now implemented with GCP GCS Client instead of apitools ([#25676](https://github.com/apache/beam/issues/25676))
-* 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))
* 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)).
Review Comment:
LowCardinality will go to 2.53.0 but Tuples does not, as 2.53.0 is already cut
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/ClickHouseIO.java:
##########
@@ -475,6 +477,15 @@ abstract static class Builder<T> {
}
}
+ private static String tuplePreprocessing(String payload) {
+ List<String> l =
+ Arrays.stream(payload.trim().split(","))
+ .map(s -> s.trim().replaceAll(" +", "':;"))
+ .collect(Collectors.toList());
+ String content =
+ String.join(",", l).trim().replaceAll("Tuple\\(", "Tuple('").replaceAll(",", ",'");
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "erube (via GitHub)" <gi...@apache.org>.
erube commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1488251709
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -111,6 +112,14 @@ public static Schema.FieldType getEquivalentFieldType(ColumnType columnType) {
return Schema.FieldType.STRING;
case BOOL:
return Schema.FieldType.BOOLEAN;
+ case TUPLE:
+ List<Schema.Field> fields =
+ columnType.tupleTypes().entrySet().stream()
+ .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))
Review Comment:
Hi @mzitnik,
Shouldn't each key inside a tuple be mapped to their respective data type ? Any reason for being `DATETIME`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "wattache (via GitHub)" <gi...@apache.org>.
wattache commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1488260740
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -111,6 +112,14 @@ public static Schema.FieldType getEquivalentFieldType(ColumnType columnType) {
return Schema.FieldType.STRING;
case BOOL:
return Schema.FieldType.BOOLEAN;
+ case TUPLE:
+ List<Schema.Field> fields =
+ columnType.tupleTypes().entrySet().stream()
+ .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))
Review Comment:
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for tuple ClickHouse [beam]
Posted by "mzitnik (via GitHub)" <gi...@apache.org>.
mzitnik commented on code in PR #29715:
URL: https://github.com/apache/beam/pull/29715#discussion_r1506273978
##########
sdks/java/io/clickhouse/src/main/java/org/apache/beam/sdk/io/clickhouse/TableSchema.java:
##########
@@ -111,6 +112,14 @@ public static Schema.FieldType getEquivalentFieldType(ColumnType columnType) {
return Schema.FieldType.STRING;
case BOOL:
return Schema.FieldType.BOOLEAN;
+ case TUPLE:
+ List<Schema.Field> fields =
+ columnType.tupleTypes().entrySet().stream()
+ .map(x -> Schema.Field.of(x.getKey(), Schema.FieldType.DATETIME))
Review Comment:
@erube and @wattache, thanks for the feedback and change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org