hudi-agent commented on code in PR #18842:
URL: https://github.com/apache/hudi/pull/18842#discussion_r3301836180
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieIndexConfig.java:
##########
@@ -777,10 +778,8 @@ private void validateBucketIndexConfig() {
hoodieIndexConfig.setValue(BUCKET_INDEX_HASH_FIELD,
hoodieIndexConfig.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME));
} else {
- boolean valid = Arrays
-
.stream(hoodieIndexConfig.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME).split(","))
- .collect(Collectors.toSet())
-
.containsAll(Arrays.asList(hoodieIndexConfig.getString(BUCKET_INDEX_HASH_FIELD).split(",")));
+ List<String> recordKeyFields =
KeyGenUtils.getRecordKeyFields(hoodieIndexConfig.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME));
+ boolean valid =
recordKeyFields.containsAll(Arrays.asList(hoodieIndexConfig.getString(BUCKET_INDEX_HASH_FIELD).split(",")));
Review Comment:
🤖 The two sides of the `containsAll` are now normalized inconsistently —
`recordKeyFields` is trimmed/empty-filtered via
`KeyGenUtils.getRecordKeyFields`, but `BUCKET_INDEX_HASH_FIELD` is still a raw
`.split(",")`. If a user configures the hash field with whitespace (e.g. `"a,
b"`), `containsAll` will now incorrectly fail since the record-key side is
`["a", "b"]` but the hash side is `["a", " b"]`. Should the hash field be
normalized through `getRecordKeyFields` too?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/test/java/org/apache/hudi/common/util/TestTryWithResourcesExceptionHandling.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.util;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class TestTryWithResourcesExceptionHandling {
+
+ @Test
Review Comment:
🤖 This test file looks unrelated to the record-key parsing refactor — and as
written the test has no assertions and unconditionally throws from the
`finally` block, so it will always fail. Was it accidentally included in this
PR?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/test/java/org/apache/hudi/common/util/TestTryWithResourcesExceptionHandling.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.util;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class TestTryWithResourcesExceptionHandling {
+
+ @Test
+ void testResourceCloseExceptionIsSwallowedWhenFinallyThrows() {
+ try (ThrowingResource ignored = new ThrowingResource("resource close
failed")) {
Review Comment:
🤖 nit: this file looks like it may have been accidentally included — it
tests try-with-resources exception suppression, not record key splitting, and
the test has no assertions (`assertEquals`/`assertThrows` are imported but
never used). As written, the test also throws unconditionally from the
`finally` block, which would always make it fail.
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/configuration/OptionsResolver.java:
##########
@@ -168,10 +169,7 @@ public static String getRecordKeyStr(Configuration conf) {
*/
public static String[] getRecordKeys(Configuration conf) {
final String recordKeyStr = conf.get(FlinkOptions.RECORD_KEY_FIELD);
- if (StringUtils.isNullOrEmpty(recordKeyStr)) {
- return new String[]{};
- }
- return recordKeyStr.split(",");
+ return KeyGenUtils.getRecordKeyFields(recordKeyStr).toArray(new String[0]);
Review Comment:
🤖 Heads up — the existing `TestOptionsResolver.testGetRecordKeys` asserts
`new String[]{"uuid", " name"}` (note the leading space) for input `"uuid,
name"`. Routing through `KeyGenUtils.getRecordKeyFields` will now trim, so that
test (and `testGetBucketIndexKeys` at line 99) should break. Could you update
them to reflect the new trimming behavior?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
