haruki-830 commented on code in PR #4425:
URL: https://github.com/apache/flink-cdc/pull/4425#discussion_r3584021589
##########
flink-cdc-connect/flink-cdc-pipeline-connectors/flink-cdc-pipeline-connector-starrocks/src/main/java/org/apache/flink/cdc/connectors/starrocks/sink/StarRocksUtils.java:
##########
@@ -492,4 +494,29 @@ public static String convertInvalidTimestampDefaultValue(
return defaultValue;
}
+
+ public static String escapeSqlStringLiteral(String value) {
+ if (value == null) {
+ return null;
+ }
+ StringBuilder sb = new StringBuilder(value.length() + 8);
+ for (int i = 0; i < value.length(); i++) {
+ char c = value.charAt(i);
+ switch (c) {
+ case '\\':
+ sb.append("\\\\");
+ break;
+ case '"':
+ sb.append("\"\"");
+ break;
+ case '\n':
+ case '\r':
+ sb.append(' ');
Review Comment:
I think this may change the semantics of DEFAULT values. This helper is used
for both COMMENT and DEFAULT, but replacing LF/CR with spaces means a source
default like `line1\nline2` will become `line1 line2` in StarRocks.
StarRocks string literal parsing already supports backslash escape
sequences.So for DEFAULT values, it seems better to emit escaped SQL literals
such as `\\n` / `\\r` rather than replacing them with spaces.
##########
flink-cdc-connect/flink-cdc-pipeline-connectors/flink-cdc-pipeline-connector-starrocks/src/test/java/org/apache/flink/cdc/connectors/starrocks/sink/StarRocksMetadataApplierITCase.java:
##########
@@ -583,6 +583,145 @@ void
testMysqlDefaultTimestampValueConversionInAddColumn() throws Exception {
assertEqualsInOrder(expected, actual);
}
+ @Test
+ void testSpecialCharactersInCommentAndDefaultValueInCreateTable() throws
Exception {
+ TableId tableId =
+ TableId.tableId(
+ StarRocksContainer.STARROCKS_DATABASE_NAME,
+ StarRocksContainer.STARROCKS_TABLE_NAME);
+
+ Schema schema =
+ Schema.newBuilder()
+ .column(new PhysicalColumn("id",
DataTypes.INT().notNull(), null))
+ .column(
+ new PhysicalColumn(
+ "col_backslash",
+ DataTypes.VARCHAR(100),
+ "comment with \\ backslash",
+ "default\\value"))
+ .column(
+ new PhysicalColumn(
+ "col_quote",
+ DataTypes.VARCHAR(100),
+ "comment with \" double quote",
+ "default\"value"))
+ .column(
+ new PhysicalColumn(
+ "col_newline",
+ DataTypes.VARCHAR(100),
+ "comment with \n newline",
+ "default\nvalue"))
+ .column(
+ new PhysicalColumn(
+ "col_mixed",
+ DataTypes.VARCHAR(100),
+ "mixed \\ and \" and \n chars",
+ "mix\\ed\"val\nue"))
+ .primaryKey("id")
+ .build();
+
+ runJobWithEvents(Collections.singletonList(new
CreateTableEvent(tableId, schema)));
+
+ List<String> actual = inspectTableSchema(tableId);
+
+ List<String> expected =
+ Arrays.asList(
+ "id | int | NO | true | null",
+ "col_backslash | varchar(300) | YES | false |
default\\value",
+ "col_quote | varchar(300) | YES | false |
default\"value",
+ "col_newline | varchar(300) | YES | false | default
value",
Review Comment:
The new coverage is helpful. I wonder if the expected value for newline
defaults should preserve the original DEFAULT semantics instead of expecting it
to become a space. Otherwise the test may lock in behavior where escaping
changes the actual default value.
##########
flink-cdc-connect/flink-cdc-pipeline-connectors/flink-cdc-pipeline-connector-starrocks/src/main/java/org/apache/flink/cdc/connectors/starrocks/sink/StarRocksUtils.java:
##########
@@ -108,7 +110,7 @@ public static StarRocksTable toStarRocksTable(
.setTableKeys(schema.primaryKeys())
// use primary keys as distribution keys by default
.setDistributionKeys(schema.primaryKeys())
- .setComment(schema.comment());
+ .setComment(escapeSqlStringLiteral(schema.comment()));
Review Comment:
It looks like this escapes table comments, but the current StarRocksCatalog
create-table path does not seem to emit table comments in the generated DDL.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
