Core: Clean up TestTrackedFileAdapters and drop TrackedFileBuilder#17144
Core: Clean up TestTrackedFileAdapters and drop TrackedFileBuilder#17144gaborkaszab wants to merge 1 commit into
Conversation
gaborkaszab
commented
Jul 9, 2026
gaborkaszab
commented
- Remove the last usage of TrackedFileBuilder
- Removed usage of setters with ordinals in TestTrackedFileAdapters
- Dropped TrackedFileBuilder and its test suite
| @Test | ||
| void testDataFileAdapterDelegation() { | ||
| TrackingStruct tracking = | ||
| new TrackingStruct( |
There was a problem hiding this comment.
After this PR TrackingBuilder has no caller in core/src/main: its only production call sites are inside TrackedFileBuilder (build(), deleted(), replaced()), all deleted here. The only reference left anywhere is TestTrackingBuilder, which is the same "only tests use it" position that motivates dropping TrackedFileBuilder, and @rdblue's note on #16964 covers it too ("stop using the builders that are already present").
Either drop TrackingBuilder and TestTrackingBuilder in this PR, or say in the description that they follow separately - #17145 is currently investing in TestTrackingBuilder in the opposite direction.
There was a problem hiding this comment.
First let's see if @stevenzwu 's wrapper PR could leverage TrackingBuilder. If not, we can drop it.
| null, | ||
| null); | ||
| tracking.setManifestLocation(MANIFEST_LOCATION); | ||
| tracking.set(MANIFEST_POS_ORDINAL, MANIFEST_POS); |
There was a problem hiding this comment.
The description says ordinal setters were removed from this file, but tracking.set(MANIFEST_POS_ORDINAL, MANIFEST_POS) stays at three call sites (here, line 153, line 238). That is correct - _pos has no setter and is not in Tracking.schema(), which is what @rdblue concluded on #17041 - but the description reads as if it were gone.
What did get removed is // manifestPos is appended after the tracking schema fields by the manifest reader., the only line explaining why Tracking.schema().fields().size() is the right index for it. @anoopj pointed at this file as the reference for that pattern on #17041; worth keeping the comment on line 62.
There was a problem hiding this comment.
I didn't like that comment after the other ordinal variables are gone. Added a rephrased comment.
| .splitOffsets(ImmutableList.of(50L, 100L)) | ||
| .build(); | ||
| populateTrackingFields(file); | ||
| new TrackedFileStruct( |
There was a problem hiding this comment.
TrackedFileBuilder still has open consumers. #16936 ("V4 write direction wrappers") calls TrackedFileBuilder.data(...), .equalityDelete(...) and .explicitTracking(...) from its writer wrappers, and it adds the explicitTracking factory to this class, so it extends TrackedFileBuilder rather than only consuming it. #16867 builds its manifest-adapter test fixtures on .dataManifest(...) / .deleteManifest(...).
Both PRs were updated after @rdblue's "stop using the builders that are already present" note on #16964, so neither is adapted yet. Worth cross-linking this PR from #16936 and #16867 so the rework happens before this lands - for #16936 that is a rewrite, not a rebase.
There was a problem hiding this comment.
Those usages will be gone.
| TrackedFileStruct file = dummyTrackedFile(FileContent.DATA); | ||
| file.set(SPEC_ID_ORDINAL, 99); | ||
| TrackedFileStruct file = | ||
| new TrackedFileStruct( |
There was a problem hiding this comment.
dummyTrackedFile two methods below (line 472) already builds exactly this file, and it is still used by the rejection and spec-resolution tests. This call site, testSpecIdMismatchThrows and testNullTrackingReturnsNullTrackingFields now hand-roll the 16-arg constructor with eleven null arguments to vary a single field, so the same fixture is expressed two different ways in one file.
An overload such as dummyTrackedFile(FileContent contentType, Integer specId) plus one for the deletion-vector case keeps these back to a one-liner without reintroducing a builder. The "values visible at the call site" argument @amogh-jahagirdar made on #17035 applies to TestTrackedFileStruct, where the struct's own CRUD is under test; here the file is plumbing for the adapter and the eleven visible values are all null.
There was a problem hiding this comment.
I disagree with this. Introducing different variations of functions like dummyTrackedFile goes against the effort we are following now to clean up tests. TrackedFileStruct is an essential input to the adapter tests and understanding its params is required to understand the test.
- Remove the last usage of TrackedFileBuilder - Removed usage of setters with ordinals in TestTrackedFileAdapters - Dropped TrackedFileBuilder and its test suite
gaborkaszab
force-pushed
the
main_drop_trackedfilebuilder
branch
from
797fa02 to
4890861
Compare
2 participants
