Refactor Run modules to use modern record extensions#830
Open
mwihoti wants to merge 4 commits intoIntersectMBO:mainfrom
Open
Refactor Run modules to use modern record extensions#830mwihoti wants to merge 4 commits intoIntersectMBO:mainfrom
mwihoti wants to merge 4 commits intoIntersectMBO:mainfrom
Conversation
Contributor
Author
|
Hi @jorisdral , I just pushed an update for Run module with the new record language extensions, I'm moving next to Index |
jorisdral
requested changes
Mar 25, 2026
Collaborator
There was a problem hiding this comment.
Thank you very much for the contribution, I like the new field names for the Run and RunReader module 😄.
Two high-level comments:
- In modules where we refactor field names (
Run/RunReader), we should enableDuplicateRecordFields,NoFieldSelectors, andOverloadedRecordDot. In other modules where we only want to use the.fieldsyntax, it should be fine to only enableOverloadedRecordDot, so let's change that so that we only use the.fieldsyntax on values of typeRunandRunReader. That probably makes the diff of this PR also smaller - There are some residual parentheses here and there. In particular, we don't need parentheses around syntax like
value.field
Comment on lines
+14
to
+15
| Run (Run, index, hasFS, hasBlockIO, dataCaching, | ||
| blobFile, bloomFilter, kOpsFile, fsPaths, numEntries, refCounter) |
Collaborator
There was a problem hiding this comment.
Suggested change
| Run (Run, index, hasFS, hasBlockIO, dataCaching, | |
| blobFile, bloomFilter, kOpsFile, fsPaths, numEntries, refCounter) | |
| Run (Run, index, hasFS, hasBlockIO, dataCaching, | |
| blobFile, bloomFilter, kOpsFile) |
Collaborator
There was a problem hiding this comment.
The fsPaths, numEntries, and refCounter fields are not exported on purpose. We should keep them internal
Comment on lines
+106
to
+112
| rnf r = | ||
| rnf r.numEntries `seq` rwhnf r.refCounter `seq` rnf r.fsPaths `seq` | ||
| rnf r.bloomFilter `seq` rnf r.index `seq` rnf r.kOpsFile `seq` | ||
| rnf r.blobFile `seq` rnf r.dataCaching `seq` rwhnf r.hasFS `seq` rwhnf r.hasBlockIO | ||
|
|
||
|
|
||
|
|
Collaborator
There was a problem hiding this comment.
This was written like this before:
rnf (Run a b c d e f g h i j) = ...I think the previous definition was better, because the explicit pattern match will result in compiler errors if we forget to use one of the fields (and forgetting to use one of the fields is bad for performance tests)
|
|
||
| sizeInPages :: Ref (Run m h) -> NumPages | ||
| sizeInPages (DeRef run) = Index.sizeInPages (runIndex run) | ||
| sizeInPages (DeRef run) = Index.sizeInPages (run.index) |
Collaborator
There was a problem hiding this comment.
There a number of cases like this where there are residual parentheses even though they are not necessary anymore:
Suggested change
| sizeInPages (DeRef run) = Index.sizeInPages (run.index) | |
| sizeInPages (DeRef run) = Index.sizeInPages run.index |
Let's make sure that we remove obsolete parentheses in the rest of PR as well
Comment on lines
+237
to
+248
| (\refCounter -> Run { | ||
| numEntries = runNumEntries | ||
| , refCounter = refCounter | ||
| , fsPaths = runRunFsPaths | ||
| , bloomFilter = runFilter | ||
| , index = runIndex | ||
| , kOpsFile = runKOpsFile | ||
| , blobFile = runBlobFile | ||
| , dataCaching = runRunDataCaching | ||
| , hasFS = runHasFS | ||
| , hasBlockIO = runHasBlockIO | ||
| }) |
Collaborator
There was a problem hiding this comment.
Suggested change
| (\refCounter -> Run { | |
| numEntries = runNumEntries | |
| , refCounter = refCounter | |
| , fsPaths = runRunFsPaths | |
| , bloomFilter = runFilter | |
| , index = runIndex | |
| , kOpsFile = runKOpsFile | |
| , blobFile = runBlobFile | |
| , dataCaching = runRunDataCaching | |
| , hasFS = runHasFS | |
| , hasBlockIO = runHasBlockIO | |
| }) | |
| (\refCounter -> Run { | |
| numEntries = runNumEntries | |
| , refCounter = refCounter | |
| , fsPaths = runRunFsPaths | |
| , bloomFilter = runFilter | |
| , index = runIndex | |
| , kOpsFile = runKOpsFile | |
| , blobFile = runBlobFile | |
| , dataCaching = runRunDataCaching | |
| , hasFS = runHasFS | |
| , hasBlockIO = runHasBlockIO | |
| }) |
| , dataCaching = runRunDataCaching | ||
| , hasFS = fs | ||
| , hasBlockIO = hbio | ||
|
|
Comment on lines
+6
to
+8
| {-# LANGUAGE DuplicateRecordFields #-} | ||
| {-# LANGUAGE NoFieldSelectors #-} | ||
| {-# LANGUAGE OverloadedRecordDot #-} |
Collaborator
There was a problem hiding this comment.
Suggested change
| {-# LANGUAGE DuplicateRecordFields #-} | |
| {-# LANGUAGE NoFieldSelectors #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} |
Comment on lines
+1
to
+6
| {-# LANGUAGE CPP #-} | ||
| {-# LANGUAGE DuplicateRecordFields #-} | ||
| {-# LANGUAGE MagicHash #-} | ||
| {-# LANGUAGE NoFieldSelectors #-} | ||
| {-# LANGUAGE OverloadedRecordDot #-} | ||
| {-# LANGUAGE UnboxedTuples #-} |
Collaborator
There was a problem hiding this comment.
Suggested change
| {-# LANGUAGE CPP #-} | |
| {-# LANGUAGE DuplicateRecordFields #-} | |
| {-# LANGUAGE MagicHash #-} | |
| {-# LANGUAGE NoFieldSelectors #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} | |
| {-# LANGUAGE UnboxedTuples #-} | |
| {-# LANGUAGE CPP #-} | |
| {-# LANGUAGE MagicHash #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} | |
| {-# LANGUAGE UnboxedTuples #-} |
Comment on lines
+2
to
+4
| {-# LANGUAGE DuplicateRecordFields #-} | ||
| {-# LANGUAGE NoFieldSelectors #-} | ||
| {-# LANGUAGE OverloadedRecordDot #-} |
Collaborator
There was a problem hiding this comment.
Suggested change
| {-# LANGUAGE DuplicateRecordFields #-} | |
| {-# LANGUAGE NoFieldSelectors #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} |
Comment on lines
+1
to
+3
| {-# LANGUAGE DuplicateRecordFields #-} | ||
| {-# LANGUAGE NoFieldSelectors #-} | ||
| {-# LANGUAGE OverloadedRecordDot #-} |
Collaborator
There was a problem hiding this comment.
Suggested change
| {-# LANGUAGE DuplicateRecordFields #-} | |
| {-# LANGUAGE NoFieldSelectors #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} |
Comment on lines
+2
to
+4
| {-# LANGUAGE DuplicateRecordFields #-} | ||
| {-# LANGUAGE NoFieldSelectors #-} | ||
| {-# LANGUAGE OverloadedRecordDot #-} |
Collaborator
There was a problem hiding this comment.
Suggested change
| {-# LANGUAGE DuplicateRecordFields #-} | |
| {-# LANGUAGE NoFieldSelectors #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} | |
| {-# LANGUAGE OverloadedRecordDot #-} |
Contributor
Author
|
Thanks, I will work on the changes |
extension scope
- Remove unnecessary parentheses around dot-access expressions (value.field)
- Only enable DuplicateRecordFields/NoFieldSelectors in Run and RunReader
where field names were actually refactored; other modules get
OverloadedRecordDot only
mwihoti
force-pushed
the
feature/811-modern-record-extensions
branch
from
343017b to
5823d35
Compare
Snapshot.hs: use exported runFsPaths accessor instead of internal r.fsPaths
Unsafe.hs: restore parens around function applications
Config/Override.hs: fix corrupted override instance for MergeBatchSize
Benchmarks: update to dot syntax with renamed fields (bloomFilter, index,
kOpsFile) and add OverloadedRecordDot pragma to both benchmark files
Contributor
Author
|
Hi @jorisdral I've updated changes based on the feedback |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Following the successful pilot with Internal.Arena, this PR refactors the Run module and several related components to use the modern record language extensions.
changes
cabal build lsm-tree:lib:core -- Builds successfully
cabal test lsm-tree-test --pattern "Run" -- 129/129 tests pass
cabal test all -- Verified for compilation and linking
link
to the issue.
Checklist