Copilot commented on code in PR #10389:
URL: https://github.com/apache/seatunnel/pull/10389#discussion_r2719396417
##########
seatunnel-api/src/main/java/org/apache/seatunnel/api/sink/TablePlaceholderProcessor.java:
##########
@@ -190,6 +202,12 @@ public static ReadonlyConfig replaceTablePlaceholder(
+ "}")) {
strValue = replaceTableFieldNames(strValue,
table.getTableSchema());
listValue =
Arrays.asList(strValue.split(FIELD_DELIMITER));
+ } else if (strValue.equals(
+ "${"
+ +
TablePlaceholder.REPLACE_PARTITION_KEYS_KEY
+ .getPlaceholder()
+ + "}")) {
+ listValue = new
ArrayList<>(table.getPartitionKeys());
Review Comment:
In the List-valued placeholder branch, `${partition_keys}` is always
replaced with `table.getPartitionKeys()` (which can be an empty list when
upstream schema doesn’t specify partition_keys). This is inconsistent with the
String placeholder behavior (which leaves the placeholder unchanged when
partition keys are empty) and with other placeholders like
`${primary_key}`/`${unique_key}` (which remain unresolved when metadata is
missing). Consider only performing this replacement when partition keys are
non-empty, or otherwise leaving the placeholder as-is / honoring a default
expression, to avoid silently converting a missing metadata value into an empty
list.
```suggestion
List<String> partitionKeys =
table.getPartitionKeys();
if (partitionKeys != null &&
!partitionKeys.isEmpty()) {
listValue = new ArrayList<>(partitionKeys);
}
```
##########
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-iceberg-e2e/src/test/java/org/apache/seatunnel/e2e/connector/iceberg/IcebergSinkIT.java:
##########
@@ -127,8 +154,13 @@ private List<Record> loadIcebergTable() {
IcebergTableLoader tableLoader =
IcebergTableLoader.create(new
IcebergSourceConfig(ReadonlyConfig.fromMap(configs)));
tableLoader.open();
+ return tableLoader.loadTable();
Review Comment:
`loadIcebergTableObject()` opens an `IcebergTableLoader` (which implements
`Closeable`) but never closes it. Over repeated calls/tests this can leak
resources (catalog/file handles) inside the container JVM. Consider using
try-with-resources around the loader (or closing it in a finally block) after
`loadTable()` returns.
--
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]
