You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2025/12/16 08:55:25 UTC
[PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
snuyanzin opened a new pull request, #27347:
URL: https://github.com/apache/flink/pull/27347
## What is the purpose of the change
The issue is that after FLINK-38702 DDLs with duplicated keys in `WITH` started to fail
This PR makes this behavior a bit more relaxed to allow to override with last value (as was before)
## Brief change log
SqlParseUtil
## Verifying this change
SqlDdlToOperationConverterTest.java
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? ( no)
- If yes, how is the feature documented? (not applicable)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27347:
URL: https://github.com/apache/flink/pull/27347#discussion_r2622401135
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/SqlParseUtils.java:
##########
@@ -87,7 +87,11 @@ public static Map<String, String> extractMap(@Nullable SqlNodeList propList) {
}
return propList.getList().stream()
.map(p -> (SqlTableOption) p)
- .collect(Collectors.toMap(k -> k.getKeyString(), SqlTableOption::getValueString));
+ .collect(
+ Collectors.toMap(
+ k -> k.getKeyString(),
+ SqlTableOption::getValueString,
+ (v1, v2) -> v2));
Review Comment:
Since by default java throws exception about duplicated keys...
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #27347:
URL: https://github.com/apache/flink/pull/27347#issuecomment-3659481233
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "94edfe1f1a549943ac1b1626949a60b2ba4a63d2",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "94edfe1f1a549943ac1b1626949a60b2ba4a63d2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 94edfe1f1a549943ac1b1626949a60b2ba4a63d2 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27347:
URL: https://github.com/apache/flink/pull/27347#discussion_r2622611435
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlDdlToOperationConverterTest.java:
##########
@@ -1117,28 +1117,33 @@ void testAlterTable() throws Exception {
// test alter table options
checkAlterNonExistTable("alter table %s nonexistent set ('k1' = 'v1', 'K2' = 'V2')");
Operation operation =
- parse("alter table if exists cat1.db1.tb1 set ('k1' = 'v1', 'K2' = 'V2')");
+ parse(
+ "alter table if exists cat1.db1.tb1 set ('k1' = 'v1', 'k2' = 'v2', 'K2' = 'V1', 'K2' = 'V2')");
Map<String, String> expectedOptions = new HashMap<>();
expectedOptions.put("connector", "dummy");
expectedOptions.put("k", "v");
expectedOptions.put("k1", "v1");
+ expectedOptions.put("k2", "v2");
expectedOptions.put("K2", "V2");
assertAlterTableOptions(
operation,
expectedIdentifier,
expectedOptions,
- Arrays.asList(TableChange.set("k1", "v1"), TableChange.set("K2", "V2")),
- "ALTER TABLE IF EXISTS cat1.db1.tb1\n SET 'k1' = 'v1',\n SET 'K2' = 'V2'");
+ Set.of(
Review Comment:
Here it in fact doesn't matter
probably the only place where it does matter for table changes is chain modification for table columns since we can change positions there like put it after or before that column which should be first declared.
Anyway changed back for now
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27347:
URL: https://github.com/apache/flink/pull/27347#discussion_r2622382111
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/catalog/SqlAlterCatalogOptions.java:
##########
@@ -55,13 +54,7 @@ public SqlNodeList getPropertyList() {
}
public Map<String, String> getProperties() {
- return propertyList.stream()
- .map(p -> (SqlTableOption) p)
- .collect(
- Collectors.toMap(
- SqlTableOption::getKeyString,
- SqlTableOption::getValueString,
- (option1, option2) -> option2));
Review Comment:
probably one missing piece from prev refactoring
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
Posted by "AHeise (via GitHub)" <gi...@apache.org>.
AHeise commented on code in PR #27347:
URL: https://github.com/apache/flink/pull/27347#discussion_r2622426060
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlDdlToOperationConverterTest.java:
##########
@@ -1117,28 +1117,33 @@ void testAlterTable() throws Exception {
// test alter table options
checkAlterNonExistTable("alter table %s nonexistent set ('k1' = 'v1', 'K2' = 'V2')");
Operation operation =
- parse("alter table if exists cat1.db1.tb1 set ('k1' = 'v1', 'K2' = 'V2')");
+ parse(
+ "alter table if exists cat1.db1.tb1 set ('k1' = 'v1', 'k2' = 'v2', 'K2' = 'V1', 'K2' = 'V2')");
Map<String, String> expectedOptions = new HashMap<>();
expectedOptions.put("connector", "dummy");
expectedOptions.put("k", "v");
expectedOptions.put("k1", "v1");
+ expectedOptions.put("k2", "v2");
expectedOptions.put("K2", "V2");
assertAlterTableOptions(
operation,
expectedIdentifier,
expectedOptions,
- Arrays.asList(TableChange.set("k1", "v1"), TableChange.set("K2", "V2")),
- "ALTER TABLE IF EXISTS cat1.db1.tb1\n SET 'k1' = 'v1',\n SET 'K2' = 'V2'");
+ Set.of(
Review Comment:
Do we need to switch to set here because we don't retain the order anymore on the low-level? (We never did on high level)
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/SqlParseUtils.java:
##########
@@ -87,7 +87,11 @@ public static Map<String, String> extractMap(@Nullable SqlNodeList propList) {
}
return propList.getList().stream()
.map(p -> (SqlTableOption) p)
- .collect(Collectors.toMap(k -> k.getKeyString(), SqlTableOption::getValueString));
+ .collect(
+ Collectors.toMap(
+ k -> k.getKeyString(),
+ SqlTableOption::getValueString,
+ (v1, v2) -> v2));
Review Comment:
Should we add a WARN?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #27347:
URL: https://github.com/apache/flink/pull/27347
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-38802][table] DDL with duplicated keys in table options should not fail [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #27347:
URL: https://github.com/apache/flink/pull/27347#discussion_r2622533189
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/SqlParseUtils.java:
##########
@@ -87,7 +87,11 @@ public static Map<String, String> extractMap(@Nullable SqlNodeList propList) {
}
return propList.getList().stream()
.map(p -> (SqlTableOption) p)
- .collect(Collectors.toMap(k -> k.getKeyString(), SqlTableOption::getValueString));
+ .collect(
+ Collectors.toMap(
+ k -> k.getKeyString(),
+ SqlTableOption::getValueString,
+ (v1, v2) -> v2));
Review Comment:
added logging
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org