refactor: Rename dataFile/df variables to baseFile/bf - #19944
nanjeshramesh wants to merge 2 commits into
Conversation
HoodieBaseFile was previously named HoodieDataFile, and many call sites
across the codebase still name variables, parameters, and lambda
arguments of this type dataFile or df rather than baseFile or bf,
per the terminology convergence the cWiki design and arch page calls
for.
This renames every local variable, field, method parameter, and
lambda argument whose declared or inferred type is HoodieBaseFile (or
a direct List/Stream/Option/Collection of it) from dataFile/df to
baseFile/bf, across 35 files in hudi-common, hudi-hadoop-common,
hudi-hadoop-mr, hudi-sync/hudi-hive-sync, hudi-client-common,
hudi-spark-client, hudi-java-client, hudi-flink-client, and
hudi-spark-datasource.
Deliberately out of scope:
- Method names such as HoodieFileGroup#getLatestDataFile and
TestHoodieBaseFile#newDataFile are unchanged. Renaming a method,
especially a public one, is a materially different, higher-risk
change than renaming a local variable or parameter, and the issue's
own wording ("all variables of this type") does not ask for it.
- Identifiers that are not of type HoodieBaseFile even though they
contain "dataFile" in the name, for example dataFileName (String),
skipCreatingDataFile (boolean), and the several FileSlice-typed
orphan/inflightFileSliceWithDataFile locals in
TestHoodieTableFileSystemView. Each was checked individually against
its actual declared or inferred type before being included or
excluded.
- df/dataFile identifiers that are unrelated Spark DataFrames or other
types entirely, such as StreamSync's transformed.map(df -> ...) and
IncrementalTimelineSyncFileSystemView's
deltaFileGroups.stream().map(df -> df.getTimeline()), the latter
being a HoodieFileGroup, not a HoodieBaseFile, despite sitting one
line away from two genuine HoodieBaseFile df usages that were
renamed.
An earlier attempt at this same issue (apache#1650) was opened in 2022 and
closed unmerged after review stalled; a good number of the dataFile/df
sites it touched have since been renamed independently in other
changes, which is why this diff is smaller in file count than that
one.
Pure rename, no behavior change: 281 insertions, 281 deletions.
Verified by test-compiling every touched module (hudi-common,
hudi-hadoop-common, hudi-hadoop-mr, hudi-hive-sync, hudi-client-common,
hudi-spark-client, hudi-java-client, hudi-flink-client,
hudi-spark-datasource/hudi-spark and its spark3.5 dependency chain)
against JDK 11 with zero compile errors.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR renames HoodieBaseFile-typed locals, fields, parameters, and lambda arguments from dataFile/df to baseFile/bf across hudi-common, hudi-client-common, the Spark/Flink/Java client tables, hadoop-common, hive-sync, and related tests, leaving method names and non-HoodieBaseFile identifiers untouched. The changes are pure identifier substitutions with no argument reordering or behavior change, the HoodieCompactionHandler#handleUpdate parameter rename is applied consistently across implementations, and the touched modules test-compile cleanly at the PR head. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Mostly mechanical rename, but the new names aren't consistent across files — some spots use the full baseFile while others use the terse bf, which reintroduces the same ambiguity the PR is trying to remove.
cc @yihua
| if (fileSliceOptional.isPresent()) { | ||
| FileSlice fs = fileSliceOptional.get(); | ||
| Option<HoodieBaseFile> df = fs.getBaseFile(); | ||
| Option<HoodieBaseFile> bf = fs.getBaseFile(); |
There was a problem hiding this comment.
🤖 nit: everywhere else in this PR dataFile/df is renamed to the full baseFile, but here it becomes the terse bf. Could you use baseFile for consistency with the rest of the rename?
|
|
||
| sizesMap.forEach((k, v) -> { | ||
| HoodieBaseFile df = TestHoodieBaseFile.newDataFile(k); | ||
| HoodieBaseFile bf = TestHoodieBaseFile.newDataFile(k); |
There was a problem hiding this comment.
🤖 nit: this uses bf while sibling test/util classes in the same PR (e.g. TestCleanerInsertAndCleanByVersions) use the full baseFile — worth aligning on one convention across the PR.
danny0405
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19944 +/- ##
============================================
- Coverage 80.23% 80.05% -0.19%
- Complexity 34747 35051 +304
============================================
Files 2546 2548 +2
Lines 142608 143949 +1341
Branches 17362 17719 +357
============================================
+ Hits 114419 115233 +814
- Misses 20277 20762 +485
- Partials 7912 7954 +42
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@nanjeshramesh Please address the nits. |
Addresses review nits: the rename previously used the abbreviated bf in a number of lambda parameters and local variables while using the full baseFile elsewhere. Converts every remaining bf introduced by this PR to baseFile so the whole change uses one convention. Files touched: CompactionAdminClient, AbstractTableFileSystemView, IncrementalTimelineSyncFileSystemView, RocksDbBasedFileSystemView, TestCleanerInsertAndCleanByVersions, TestHoodieCompactionStrategy, TestHoodieTableFileSystemView, TestCompactionUtils. Verified by test-compiling hudi-common, hudi-hadoop-common, hudi-client-common, and hudi-spark-client against JDK 11 with no errors, including no lambda-parameter shadowing conflicts.
|
Addressed in e0ba874. Both hudi-agent nits (CompactionAdminClient and TestHoodieCompactionStrategy) flagged the same inconsistency: I'd used the abbreviated On the Codecov patch-coverage note: those flagged lines are all pure renames of parameters and locals that were equally uncovered before and after, so the project-wide delta is -0.01% and no behavior or coverage actually changed. Happy to add anything if you'd still like it, but I didn't think new tests for a rename made sense. |
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR renames dataFile/df identifiers whose type is HoodieBaseFile (or collections/Options of it) to baseFile across hudi-common, the client modules, the file-system-view code, and related test utilities, without touching method names or non-HoodieBaseFile identifiers. The diff is a pure identifier rename with no shadowing of enclosing locals, and the affected modules compile cleanly on the PR head. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Mechanical rename looks clean and consistent; one minor leftover naming inconsistency noted below.
cc @yihua
| private static HoodieBaseFile refreshFileStatus(Configuration conf, HoodieBaseFile dataFile) { | ||
| StoragePath dataPath = dataFile.getPathInfo().getPath(); | ||
| private static HoodieBaseFile refreshFileStatus(Configuration conf, HoodieBaseFile baseFile) { | ||
| StoragePath dataPath = baseFile.getPathInfo().getPath(); |
There was a problem hiding this comment.
🤖 nit: since the param is now baseFile, it might be worth renaming dataPath to baseFilePath too so the method uses one term throughout.
5 participants
Describe the issue this Pull Request addresses
Closes #14528.
HoodieBaseFile was previously named HoodieDataFile. Per the cWiki design and arch page, we should converge on one name, and many call sites across the codebase still name variables of this type dataFile or df rather than baseFile or bf.
Summary and Changelog
Renames every local variable, field, method parameter, and lambda argument whose declared or inferred type is HoodieBaseFile (or a direct List/Stream/Option/Collection of it) from dataFile/df to baseFile/bf, across 35 files in hudi-common, hudi-hadoop-common, hudi-hadoop-mr, hudi-sync/hudi-hive-sync, hudi-client-common, hudi-spark-client, hudi-java-client, hudi-flink-client, and hudi-spark-datasource.
Deliberately out of scope:
HoodieFileGroup#getLatestDataFileandTestHoodieBaseFile#newDataFileare unchanged. Renaming a method, especially a public one, is a materially different, higher risk change than renaming a local variable or parameter, and the issue's own wording ("all variables of this type") does not ask for it.dataFileName(String),skipCreatingDataFile(boolean), and the several FileSlice-typedorphan/inflightFileSliceWithDataFilelocals inTestHoodieTableFileSystemView. Each was checked individually against its actual declared or inferred type before being included or excluded.df/dataFileidentifiers that are unrelated Spark DataFrames or other types entirely, such asStreamSync'stransformed.map(df -> ...)andIncrementalTimelineSyncFileSystemView'sdeltaFileGroups.stream().map(df -> df.getTimeline()), the latter being a HoodieFileGroup, not a HoodieBaseFile, despite sitting one line away from two genuine HoodieBaseFiledfusages that were renamed.An earlier attempt at this same issue (#1650) was opened in 2022 and closed unmerged after review stalled. A good number of the dataFile/df sites it touched have since been renamed independently in other changes, which is why this diff is smaller in file count than that one.
Pure rename, no behavior change: 281 insertions, 281 deletions.
Impact
None. Internal identifier renaming only, no public API, storage format, or behavior change.
Risk Level
low
Verified by test-compiling every touched module (hudi-common, hudi-hadoop-common, hudi-hadoop-mr, hudi-hive-sync, hudi-client-common, hudi-spark-client, hudi-java-client, hudi-flink-client, hudi-spark-datasource/hudi-spark and its spark3.5 dependency chain) against JDK 11 with zero compile errors.
Documentation Update
none
Contributor's checklist