lokeshj1703 commented on PR #18076:
URL: https://github.com/apache/hudi/pull/18076#issuecomment-4854056827
Follow-up to my earlier review (apologies for the garbled headers above —
shell expansion ate the filenames).
**`CloudSourceConfig.java` — wrong `sinceVersion`**
`sinceVersion("1.4.0")` should be `"1.3.0"` — the current pom is
`1.3.0-SNAPSHOT`. Other recently-added configs in the file
(`META_EVENTS_PER_PARTITION`, `CLOUD_INCREMENTAL_MERGE_SCHEMA`) correctly use
the version they were introduced in. Inline comment left on the specific line.
**`IncrSourceHelper.java` — missing validation for `numFilesLimit`**
The comment on the `collectedRows.isEmpty()` fallback says *"numFilesLimit
is assumed to be >= 1 (validated at config layer)"*, but there is no such
validation in `CloudDataFetcher`. If a user sets this to 0,
`cumulativeCount.leq(0)` is always false so `collectedRows` is always empty —
the fallback then silently takes the first file via
`cumulativeCount.equalTo(1)`, ignoring the limit. Suggest adding
`ValidationUtils.checkArgument(numFilesLimit >= 1, ...)` in `CloudDataFetcher`
right after reading the config.
**Note on the fallback path**: since `row_number()` starts at 1 and
`numFilesLimit >= 1` is enforced, the first file's `cumulativeCount = 1` always
satisfies `leq(numFilesLimit)`. So the fallback (`collectedRows.isEmpty()`) is
exclusively triggered by the byte limit, never the files limit. The comment
saying *"numFilesLimit is assumed to be >= 1"* in the fallback block should
reflect this — e.g. *"The first file itself exceeds the byte limit; take it
unconditionally to guarantee progress (numFilesLimit >= 1 is enforced at the
call site)"*.
--
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]
