Refactor Run modules to use modern record extensions by mwihoti · Pull Request #830 · IntersectMBO/lsm-tree

mwihoti · 2026-03-25T09:29:24Z

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

Record Modernization: Refactored the Run record fields to use shorter, prefix-free names (e.g., runIndex - index, runFilter - bloomFilter), which significantly improves the developer experience with dot notation.
Test Suite Cleanup: Extensively updated the StateMachine and Lookup test suites to use dot notation instead of traditional field accessors.

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

Read our contribution guidelines at CONTRIBUTING.md, and make sure that this PR complies with the guidelines.

mwihoti · 2026-03-25T09:38:56Z

Hi @jorisdral , I just pushed an update for Run module with the new record language extensions, I'm moving next to Index

jorisdral

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 enable DuplicateRecordFields, NoFieldSelectors, and OverloadedRecordDot. In other modules where we only want to use the .field syntax, it should be fine to only enable OverloadedRecordDot, so let's change that so that we only use the .field syntax on values of type Run and RunReader. 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

mwihoti · 2026-03-26T10:09:30Z

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

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

mwihoti · 2026-04-03T08:06:18Z

Hi @jorisdral I've updated changes based on the feedback

mwihoti · 2026-04-15T02:21:42Z

Hello, @jorisdral I'would like to proceed next to index, could you check if what I've done aligns well

jorisdral

@mwihoti thanks very much! I think the changes look good now. I have some remaining comments but they are only about formatting. Once those are resolved, we can merge this PR 😃

jorisdral · 2026-04-15T08:53:49Z

+    let blooms  = V.map (\(DeRef r) -> r.bloomFilter   ) runs
+        indexes = V.map (\(DeRef r) -> r.index    ) runs
+        handles = V.map (\(DeRef r) -> r.kOpsFile ) runs


Suggested change

let blooms = V.map (\(DeRef r) -> r.bloomFilter ) runs

indexes = V.map (\(DeRef r) -> r.index ) runs

handles = V.map (\(DeRef r) -> r.kOpsFile ) runs

let blooms = V.map (\(DeRef r) -> r.bloomFilter) runs

indexes = V.map (\(DeRef r) -> r.index ) runs

handles = V.map (\(DeRef r) -> r.kOpsFile ) runs

jorisdral · 2026-04-15T08:54:09Z

+      env ( pure ( V.map (\(DeRef r) -> r.bloomFilter   ) rs
+                 , V.map (\(DeRef r) -> r.index    ) rs
+                 , V.map (\(DeRef r) -> r.kOpsFile ) rs


Suggested change

env ( pure ( V.map (\(DeRef r) -> r.bloomFilter ) rs

, V.map (\(DeRef r) -> r.index ) rs

, V.map (\(DeRef r) -> r.kOpsFile ) rs

env ( pure ( V.map (\(DeRef r) -> r.bloomFilter) rs

, V.map (\(DeRef r) -> r.index ) rs

, V.map (\(DeRef r) -> r.kOpsFile ) rs

jorisdral · 2026-04-15T08:55:46Z

+    rnf numEntries `seq` rwhnf refCounter `seq` rnf fsPaths `seq`
+    rnf bloomFilter `seq` rnf index `seq` rnf kOpsFile `seq`
+    rnf blobFile `seq` rnf dataCaching `seq` rwhnf hasFS `seq` rwhnf hasBlockIO
+


Suggested change

jorisdral · 2026-04-15T08:56:21Z

+                numEntries  =  runNumEntries
+              , refCounter  =  refCounter
+              , fsPaths     =  runRunFsPaths


Suggested change

numEntries = runNumEntries

, refCounter = refCounter

, fsPaths = runRunFsPaths

numEntries = runNumEntries

, refCounter = refCounter

, fsPaths = runRunFsPaths

jorisdral · 2026-04-15T08:57:36Z

  -> m ()
-close RunReader{..} = do
-    when (readerRunDataCaching == Run.NoCacheRunData) $
+close r = do -- Use 'r' instead of RunReader{..}


This is not resolved yet: remove the comment

jorisdral · 2026-04-15T08:58:44Z


 {-# INLINE tableSessionSalt #-}
- -- | Inherited from session for ease of access.
+ -- | Inherited from session for easenve of access.


Suggested change

-- | Inherited from session for easenve of access.

-- | Inherited from session for ease of access.

jorisdral · 2026-04-15T08:59:19Z

              !wbblobs = tableWriteBufferBlobs content
          wbblobs' <- withRollback reg (dupRef wbblobs) releaseRef
-          let runs = cachedRuns (tableCache content)
+          let runs = cachedRuns(tableCache content)


Suggested change

let runs = cachedRuns(tableCache content)

let runs = cachedRuns (tableCache content)

jorisdral · 2026-04-15T09:00:56Z

-    traceWith childTableTracer $ TraceIncrementalUnions (NE.map tableId ts)
+    childTableId <- uniqueToTableId <$> incrUniqCounter sesh.sessionUniqCounter
+    let childTableTracer = TraceTable childTableId `contramap` sesh.lsmTreeTracer
+    traceWith childTableTracer $ TraceIncrementalUnions (NE.map (\t -> t.tableId) ts)


Suggested change

traceWith childTableTracer $ TraceIncrementalUnions (NE.map (\t -> t.tableId) ts)

traceWith childTableTracer $ TraceIncrementalUnions (NE.map (.tableId) ts)

jorisdral · 2026-04-15T09:05:31Z

There are redundant parentheses around record-dot syntax in this module

jorisdral · 2026-04-15T09:07:42Z

+          (V.mapStrict (\(DeRef r) -> r.bloomFilter) runs)
+          (V.mapStrict (\(DeRef r) -> r.index    ) runs)
+          (V.mapStrict (\(DeRef r) -> r.kOpsFile) runs)


Suggested change

(V.mapStrict (\(DeRef r) -> r.bloomFilter) runs)

(V.mapStrict (\(DeRef r) -> r.index ) runs)

(V.mapStrict (\(DeRef r) -> r.kOpsFile) runs)

(V.mapStrict (\(DeRef r) -> r.bloomFilter) runs)

(V.mapStrict (\(DeRef r) -> r.index ) runs)

(V.mapStrict (\(DeRef r) -> r.kOpsFile ) runs)

mwihoti requested review from dcoutts, jorisdral and mheinzel as code owners March 25, 2026 09:29

jorisdral requested changes Mar 25, 2026

View reviewed changes

jorisdral assigned mwihoti Mar 30, 2026

mwihoti added 2 commits April 3, 2026 09:00

Refactor Run modules to use modern record extensions

2e608bd

mwihoti force-pushed the feature/811-modern-record-extensions branch from 343017b to 5823d35 Compare April 3, 2026 06:01

mwihoti added 3 commits April 3, 2026 17:11

Apply stylish-haskell formatting

6ccaf2d

apply haskell formatting

2b0d319

Remove redundant blobFile let-binding shadowed by dupRef in RunReader

6568b11

jorisdral approved these changes Apr 15, 2026

View reviewed changes

	-- \| Inherited from session for easenve of access.
	-- \| Inherited from session for ease of access.

	let runs = cachedRuns(tableCache content)
	let runs = cachedRuns (tableCache content)

	traceWith childTableTracer $ TraceIncrementalUnions (NE.map (\t -> t.tableId) ts)
	traceWith childTableTracer $ TraceIncrementalUnions (NE.map (.tableId) ts)

Conversation

mwihoti commented Mar 25, 2026

Description

changes

Checklist

Uh oh!

mwihoti commented Mar 25, 2026

Uh oh!

jorisdral left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwihoti commented Mar 26, 2026

Uh oh!

mwihoti commented Apr 3, 2026

Uh oh!

mwihoti commented Apr 15, 2026

Uh oh!

jorisdral left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jorisdral left a comment •

edited

Loading