Skip to content

refactor(common): hoist shared instant file name bodies - #19889

Open
voonhous wants to merge 1 commit into
apache:masterfrom
voonhous:dedupe-instant-file-name-generator
Open

refactor(common): hoist shared instant file name bodies#19889
voonhous wants to merge 1 commit into
apache:masterfrom
voonhous:dedupe-instant-file-name-generator

Conversation

@voonhous

Copy link
Copy Markdown
Member

Describe the issue this Pull Request addresses

InstantFileNameGeneratorV1 and InstantFileNameGeneratorV2 were byte-identical except for five members. The other 33 methods (the StringUtils.join(instantTime, EXTENSION) one-liners plus makeDeltaFileName, getCommitFromCommitFile, makeFileNameAsComplete and makeFileNameAsInflight) were maintained twice, so any new extension had to be added in both places.

Summary and Changelog

No user-visible change. Pure code motion inside hudi-common.

  • Add abstract BaseInstantFileNameGenerator in org.apache.hudi.common.table.timeline, next to the interface, holding the 33 layout-independent bodies unchanged.
  • InstantFileNameGeneratorV1 and InstantFileNameGeneratorV2 now extend it and keep only what differs per layout version:
    • getLayoutVersion
    • makeRequestedClusteringFileName / makeInflightClusteringFileName (V1 aliases the replace-commit names, V2 has dedicated clustering extensions)
    • getFileName(HoodieInstant) / getFileName(String completionTime, HoodieInstant) (V2 emits requestedTime_completionTime for completed instants)
  • The InstantFileNameGenerator interface is unchanged, DefaultInstantFileNameGenerator still extends V2, and every caller goes through the interface or TimelineLayout, so no call site changes.

The five members are left abstract rather than defaulted, so a future layout version still has to state them explicitly.

Details
  • A base class was chosen over interface default methods so out-of-repo implementers of InstantFileNameGenerator are not affected.
  • The only text edit inside a retained body is V1's comment typo 1n 0.x to In 0.x.
  • Both concrete classes keep their public no-arg constructors, so the direct constructions in TimelineLayout, TimelineArchiverV1, HoodieMetaserverBasedTimeline and the Spark tests (TestSparkReaderContextFactory, TestHoodieSparkRollback, TestColumnStatsIndex) compile as before.
  • Behaviour is pinned by the existing hudi-common timeline tests; no new tests, since no generated name changes.
  • Diffstat: 3 files changed, 205 insertions(+), 339 deletions(-)

Impact

None. The generated file names are identical for every action, state and layout version.

Risk Level

none

Documentation Update

none

Contributor's checklist

  • Read through contributor's guide
  • Enough context is provided in the sections above
  • Adequate tests were added if applicable

InstantFileNameGeneratorV1 and InstantFileNameGeneratorV2 were
byte-identical except for five members. Move the 33 shared bodies (the
StringUtils.join extension one-liners plus makeDeltaFileName,
getCommitFromCommitFile, makeFileNameAsComplete and
makeFileNameAsInflight) verbatim into a new abstract
BaseInstantFileNameGenerator in org.apache.hudi.common.table.timeline.

V1 and V2 now extend the base and keep only the layout-specific
members: getLayoutVersion, makeRequestedClusteringFileName,
makeInflightClusteringFileName (V1 aliases the replace-commit names,
V2 has dedicated clustering extensions), getFileName(HoodieInstant)
and getFileName(String, HoodieInstant) (V2 emits
requestedTime_completionTime for completed instants). These stay
abstract so a future layout version must state them explicitly.

The InstantFileNameGenerator interface, DefaultInstantFileNameGenerator
and every caller are unchanged. The only text edit inside a retained
body is V1's "1n 0.x" comment typo, now "In 0.x".

@hudi-agent hudi-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for the contribution! This PR hoists the 33 layout-independent instant file naming methods out of InstantFileNameGeneratorV1/V2 into a new abstract BaseInstantFileNameGenerator, leaving only the five layout-specific members in the concrete classes. I verified against origin/master that the two originals differed only in those five members, that the hoisted bodies are byte-identical, that all 37 interface methods remain implemented, and that nothing keys on the concrete class hierarchy. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.

cc @yihua

@github-actions github-actions Bot added the size:L PR with lines of changes in (300, 1000] label Sep 10, 2026
@danny0405

Copy link
Copy Markdown
Contributor

shoude be by design, the pattern is introducing a common interface then duplicate the impls, so that it is more easier to drop these logics when moving forward. The cost of maintaining looks okay since the evolving itself is quite slow and we just got two copies.

@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.68%. Comparing base (8af07e1) to head (4164c11).

Files with missing lines Patch % Lines
...n/table/timeline/BaseInstantFileNameGenerator.java 94.11% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19889      +/-   ##
============================================
+ Coverage     77.60%   79.68%   +2.08%     
- Complexity    33544    34388     +844     
============================================
  Files          2546     2547       +1     
  Lines        142452   142420      -32     
  Branches      17330    17330              
============================================
+ Hits         110553   113493    +2940     
+ Misses        23830    20929    -2901     
+ Partials       8069     7998      -71     
Components Coverage Δ
hudi-common 83.52% <94.44%> (+1.19%) ⬆️
hudi-client 82.27% <ø> (+3.25%) ⬆️
hudi-flink 85.59% <ø> (-0.02%) ⬇️
hudi-spark-datasource 72.72% <ø> (+6.42%) ⬆️
hudi-utilities 78.18% <ø> (ø)
hudi-cli 69.99% <ø> (ø)
hudi-hadoop 69.35% <ø> (+1.29%) ⬆️
hudi-sync 75.99% <ø> (ø)
hudi-io 81.61% <ø> (+0.14%) ⬆️
hudi-timeline-service 78.42% <ø> (ø)
hudi-cloud 80.99% <ø> (ø)
hudi-kafka-connect 53.20% <ø> (ø)
Flag Coverage Δ
common-and-other-modules 52.00% <69.44%> (+<0.01%) ⬆️
flink-integration-tests 49.13% <69.44%> (+<0.01%) ⬆️
hadoop-mr-java-client 43.96% <72.22%> (-0.01%) ⬇️
integration-tests 13.46% <30.55%> (+<0.01%) ⬆️
spark-client-hadoop-common 38.54% <94.44%> (+<0.01%) ⬆️
spark-java-tests 48.59% <91.66%> (+16.40%) ⬆️
spark-scala-tests 46.95% <88.88%> (-0.01%) ⬇️
utilities 36.85% <63.88%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...line/versioning/v1/InstantFileNameGeneratorV1.java 70.90% <100.00%> (-1.82%) ⬇️
...line/versioning/v2/InstantFileNameGeneratorV2.java 78.31% <100.00%> (-4.45%) ⬇️
...n/table/timeline/BaseInstantFileNameGenerator.java 94.11% <94.11%> (ø)

... and 277 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@voonhous

voonhous commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

shoude be by design, the pattern is introducing a common interface then duplicate the impls, so that it is more easier to drop these logics when moving forward. The cost of maintaining looks okay since the evolving itself is quite slow and we just got two copies.

I don't quite understand the argument here. If V1 needs to be dropped, the whole class can be removed even with the refactor without issue.

Also, flattening both into each class means that looking for differences can only be answered by diffing ~200 lines. With a skeletal base, the version class contains nothing but the delta, so the per-version mapping becomes more visible, not less.

This improves code readability, easier to review if we're not using LLM assisted reviews and also copy-paste errors.

@hudi-bot

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L PR with lines of changes in (300, 1000]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants