hudi-agent commented on code in PR #19245:
URL: https://github.com/apache/hudi/pull/19245#discussion_r3565773060
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/cdc/HoodieNativeLogFormatWriter.java:
##########
@@ -278,8 +278,17 @@ private void ensureAppendVersion() throws IOException {
private void closeFileWriters() throws IOException {
if (dataFileWriter != null) {
dataFileWriter.close();
- lastDataFileFormatMetadata =
writeConfig.isMetadataColumnStatsIndexEnabled()
- ? Option.ofNullable(dataFileWriter.getFileFormatMetadata()) :
Option.empty();
+ if (writeConfig.isMetadataColumnStatsIndexEnabled()) {
+ try {
+ lastDataFileFormatMetadata =
Option.ofNullable(dataFileWriter.getFileFormatMetadata());
+ } catch (UnsupportedOperationException e) {
+ // File-format metadata is an optional writer capability.
Unsupported formats can still produce valid
Review Comment:
🤖 Since `getFileFormatMetadata()`'s default contract throws
`UnsupportedOperationException` to signal "not supported," catching it here
matches the interface intent. My only worry is that a *supported* writer could
throw `UnsupportedOperationException` from some inner call for an unrelated
reason — that would get silently swallowed and quietly disable column-stats
indexing for that file with no signal. Would it be worth logging at debug/warn
here so an unexpected occurrence is at least traceable?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/cdc/HoodieNativeLogFormatWriter.java:
##########
@@ -278,8 +278,17 @@ private void ensureAppendVersion() throws IOException {
private void closeFileWriters() throws IOException {
if (dataFileWriter != null) {
dataFileWriter.close();
- lastDataFileFormatMetadata =
writeConfig.isMetadataColumnStatsIndexEnabled()
- ? Option.ofNullable(dataFileWriter.getFileFormatMetadata()) :
Option.empty();
+ if (writeConfig.isMetadataColumnStatsIndexEnabled()) {
+ try {
+ lastDataFileFormatMetadata =
Option.ofNullable(dataFileWriter.getFileFormatMetadata());
+ } catch (UnsupportedOperationException e) {
+ // File-format metadata is an optional writer capability.
Unsupported formats can still produce valid
+ // native log files; they simply do not contribute column stats for
this append.
+ lastDataFileFormatMetadata = Option.empty();
+ }
+ } else {
Review Comment:
🤖 nit: both the `catch` block (line 287) and the `else` branch set
`lastDataFileFormatMetadata = Option.empty()`. Could you initialise it to
`Option.empty()` before the `if`, and then just attempt the override inside a
`try` with an empty (commented) `catch`? That would remove the duplication
without losing the explanatory comment.
<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]
