hudi-agent commented on code in PR #18944:
URL: https://github.com/apache/hudi/pull/18944#discussion_r3382315282
##########
hudi-common/src/main/java/org/apache/hudi/common/table/log/InstantRange.java:
##########
@@ -37,6 +41,7 @@
/**
* An instant range used for incremental reader filtering.
*/
+@Getter
Review Comment:
π€ Removing the explicit `getStartInstant()`/`getEndInstant()` methods is a
breaking change β with `@Getter` on the class, Lombok derives method names from
the field names (`startInstantOpt`/`endInstantOpt`), producing
`getStartInstantOpt()`/`getEndInstantOpt()` instead. Only the
`HoodieCDCExtractor` caller was updated; other callers still use the old names
and will fail to compile, e.g.
`hudi-flink-datasource/.../HoodieSourceSplitSerializer.java` (lines 106β112,
main code) plus `TestHoodieSourceSplitSerializer`, `TestHoodieSourceSplit`,
`TestIncrementalInputSplits`, `TestStreamReadMonitoringFunction`. Could you
either add explicit `getStartInstant()`/`getEndInstant()` methods (with
`@Getter(AccessLevel.NONE)` on the two fields) or rename the fields to
`startInstant`/`endInstant` so Lombok generates the original names, and update
remaining callers if you take the rename route?
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/source/split/HoodieSourceSplitSerializer.java:
##########
@@ -103,13 +103,13 @@ public byte[] serialize(HoodieSourceSplit obj) throws
IOException {
out.writeBoolean(false);
}
- out.writeBoolean(instantRange.getStartInstant().isPresent());
- if (instantRange.getStartInstant().isPresent()) {
- out.writeUTF(instantRange.getStartInstant().get());
+ out.writeBoolean(instantRange.getStartInstantOpt().isPresent());
Review Comment:
π€ These call-site renames reflect renaming
`InstantRange.getStartInstant()`/`getEndInstant()` β
`getStartInstantOpt()`/`getEndInstantOpt()` in chunk 1. `InstantRange` is a
public abstract class in `hudi-common` (not `@PublicAPIClass`-annotated, but
still publicly reachable), and the PR description says "Public API: None" β
could you confirm this rename is acceptable for downstream integrators? If a
deprecated wrapper isn't desired, it might be worth calling out the rename in
the changelog so external consumers know to update.
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/IncrementalQueryAnalyzer.java:
##########
@@ -361,9 +360,12 @@ public IncrementalQueryAnalyzer build() {
/**
* Represents the analyzed query context.
*/
+ @AllArgsConstructor
Review Comment:
π€ Was the visibility change of `QueryContext`'s constructor intentional? It
was previously `private` (with `create()` as the public factory that wraps
StringβOption for null-safety); bare `@AllArgsConstructor` makes it public, so
callers can now bypass `create()` and pass `null` Options directly. Consider
`@AllArgsConstructor(access = AccessLevel.PRIVATE)` to preserve the original
encapsulation β the pattern used on `DefaultFileGroupRecordBufferLoader` in
this same chunk.
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableVersion.java:
##########
@@ -51,26 +59,14 @@ public enum HoodieTableVersion {
// 1.1
NINE(9, CollectionUtils.createImmutableList("1.1.0"),
TimelineLayoutVersion.LAYOUT_VERSION_2);
+ @Accessors(fluent = true)
Review Comment:
π€ nit: it might be worth adding a brief comment here explaining why only
`versionCode` uses `@Accessors(fluent = true)` while the rest of the class uses
standard `@Getter` β e.g. `// preserve existing versionCode() method name in
the public API`. The mixing of fluent and bean-style accessors on the same
class could puzzle a future reader.
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -556,7 +554,7 @@ private static void modify(HoodieStorage storage,
StoragePath metadataFolder, Pr
propsToDelete.forEach(propToDelete -> props.remove(propToDelete));
checksum = storeProperties(props, out, cfgPath);
}
- LOG.warn(String.format("%s modified to: %s (at %s)", cfgPath.getName(),
props, cfgPath.getParent()));
+ log.warn(String.format("%s modified to: %s (at %s)", cfgPath.getName(),
props, cfgPath.getParent()));
Review Comment:
π€ nit: this `log.warn` still uses `String.format` rather than SLF4J's
parameterized `{}` placeholders β could you update it to `log.warn("{} modified
to: {} (at {})", cfgPath.getName(), props, cfgPath.getParent())`? Every other
log call in this diff was correctly migrated.
<sub><i>- AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieLogBlock.java:
##########
@@ -65,32 +70,24 @@ public abstract class HoodieLogBlock {
*/
public static int version = 3;
Review Comment:
π€ The original constructor parameters used `javax.annotation.Nonnull`
(purely a static-analysis hint). Replacing them with Lombok's `@NonNull` on the
fields means the generated `@AllArgsConstructor` now inserts a runtime null
check and throws NPE on construction. This is a behavioral change (not pure
refactor): if any subclass/caller currently passes `null` for
`logBlockHeader`/`logBlockFooter`/`blockContentLocation`/`content` (even by
accident in tests), it now fails at construction. Did you verify all call sites
(including `HoodieCorruptBlock`, `HoodieDataBlock`, `HoodieDeleteBlock`,
`HoodieCommandBlock`, `HoodieHFileDataBlock`) always pass non-null `Option`
instances? If not, this could regress callers that were relying on the existing
(lenient) 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]
