manifest: move L0-specific fields from TableMetadata into l0Sublevels#5857
Conversation
|
I noticed this TODO and had Claude take a stab at it, I think it did a pretty good job. |
annrpom
left a comment
There was a problem hiding this comment.
@annrpom reviewed 8 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on RaduBerinde and xxmplus).
compaction_test.go line 2392 at r1 (raw file):
return err.Error() } _, err = strconv.Atoi(data[3:])
should we be discarding this sublevelNum?
internal/manifest/l0_sublevels_test.go line 337 at r1 (raw file):
sublevels.fileStateMap[f.TableNum] = idx idx++ _ = sublevelIdx
nit: sublevelIdx doesn't seem to be needed
TableMetadata contained four L0-specific fields (SubLevel, L0Index,
minIntervalIndex, maxIntervalIndex) that were only meaningful for L0
files, only valid for the most recent Version's l0Sublevels, and written
exclusively by l0Sublevels code. Storing them on the shared TableMetadata
struct was fragile because any code could accidentally read stale values
from a non-current Version.
This change introduces an l0FileState struct and stores per-file state in
l0Sublevels via a fileState slice (indexed by file position) and a
fileStateMap (TableNum -> index). The L0Index concept is now implicit as
the index into fileState.
L0CompactionFiles.FilesIncluded is changed from bitSet (indexed by
L0Index) to map[base.TableNum]struct{}, eliminating the need for
L0Index to be accessible outside l0Sublevels.
An L0Organizer.SubLevelOf method is added for external callers (e.g.
generateSublevelInfo in compaction_picker.go) that need to look up a
file's sublevel.
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
479254f to
11eca10
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
TFTR!
@RaduBerinde made 2 comments and resolved 1 discussion.
Reviewable status: 3 of 8 files reviewed, 1 unresolved discussion (waiting on annrpom and xxmplus).
compaction_test.go line 2392 at r1 (raw file):
Previously, annrpom (annie pompa) wrote…
should we be discarding this sublevelNum?
Added a check that it matches the current (implicit) sublevel.
TableMetadata contained four L0-specific fields (SubLevel, L0Index,
minIntervalIndex, maxIntervalIndex) that were only meaningful for L0
files, only valid for the most recent Version's l0Sublevels, and written
exclusively by l0Sublevels code. Storing them on the shared TableMetadata
struct was fragile because any code could accidentally read stale values
from a non-current Version.
This change introduces an l0FileState struct and stores per-file state in
l0Sublevels via a fileState slice (indexed by file position) and a
fileStateMap (TableNum -> index). The L0Index concept is now implicit as
the index into fileState.
L0CompactionFiles.FilesIncluded is changed from bitSet (indexed by
L0Index) to map[base.TableNum]struct{}, eliminating the need for
L0Index to be accessible outside l0Sublevels.
An L0Organizer.SubLevelOf method is added for external callers (e.g.
generateSublevelInfo in compaction_picker.go) that need to look up a
file's sublevel.
Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com