Skip to content

Conversation

@mj-palanker
Copy link
Contributor

@mj-palanker mj-palanker commented Nov 2, 2025

Summary by CodeRabbit

  • New Features
    • Added a new configuration option to control grant expansion behavior in the compaction process. Users can now enable or disable grant expansion based on their needs, with grant expansion enabled by default to maintain backward compatibility.

@mj-palanker mj-palanker requested a review from ggreer November 2, 2025 06:14
@coderabbitai
Copy link

coderabbitai bot commented Nov 2, 2025

Walkthrough

A new boolean field shouldExpandGrants was added to the Compactor struct with a corresponding public option function WithShouldExpandGrants. The Compact method now conditionally executes the grant-expansion flow based on this field's value, defaulting to true to preserve existing behavior.

Changes

Cohort / File(s) Summary
Grant expansion configurability
pkg/synccompactor/compactor.go
Added shouldExpandGrants boolean field to Compactor struct and WithShouldExpandGrants option function. Modified NewCompactor to initialize the field to true by default. Updated Compact method to conditionally skip or execute the grant-expansion sequence (empty connector creation, syncer building, Sync, and Close) based on the field value.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Compactor
    participant GrantExpansion as Grant Expansion<br/>(conditional)
    
    Caller->>Compactor: Compact()
    alt shouldExpandGrants == true
        Compactor->>GrantExpansion: Create empty connector
        GrantExpansion->>GrantExpansion: Build syncer
        GrantExpansion->>GrantExpansion: Perform Sync
        GrantExpansion->>GrantExpansion: Close
        GrantExpansion-->>Compactor: Return result/error
    else shouldExpandGrants == false
        Compactor->>Compactor: Skip grant expansion
    end
    Compactor-->>Caller: Return
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify the default behavior (true) maintains backward compatibility and doesn't introduce regressions
  • Confirm the conditional logic properly wraps all grant-expansion steps and error handling
  • Review the new public API surface and ensure the option function integrates correctly with the builder pattern
  • Assess test coverage for both true and false branches of the new flag

Suggested reviewers

  • ggreer
  • danlarkin
  • laurenleach

Poem

🐰 Hops of joy across the code,
A flag to lighten CompactØode,
Grants expand or stay compressed,
Now you choose what works the best!
Flexibility, truly blessed!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "add new option to skip grant expansion in compaction" accurately and directly describes the primary change in the changeset. The pull request introduces a new boolean field shouldExpandGrants and a public option function WithShouldExpandGrants to control whether grant expansion occurs during compaction. The title is concise, specific, and uses clear language without vague terms or unnecessary noise. It captures the main intent of the changes in a way that would allow developers scanning the commit history to quickly understand what the PR accomplishes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mjp/compact-without-expand-grants

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 349dbc2 and 3a15de6.

📒 Files selected for processing (1)
  • pkg/synccompactor/compactor.go (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/synccompactor/compactor.go (2)
pkg/sdk/empty_connector.go (1)
  • NewEmptyConnector (164-166)
pkg/sync/syncer.go (5)
  • NewSyncer (2951-2977)
  • WithC1ZPath (2858-2862)
  • WithTmpDir (2864-2868)
  • WithSyncID (2923-2927)
  • WithOnlyExpandGrants (2912-2916)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: go-test (1.25.2, windows-latest)
  • GitHub Check: go-test (1.25.2, ubuntu-latest)
  • GitHub Check: go-lint
🔇 Additional comments (1)
pkg/synccompactor/compactor.go (1)

124-157: Conditional grant expansion toggle works

The guard cleanly preserves the old flow by default while letting callers skip the expansion without touching downstream logic. Nicely done.


Comment @coderabbitai help to get the list of available commands and usage tips.

@pquerna
Copy link
Contributor

pquerna commented Nov 2, 2025

this doesn't make sense to me. you still need to expand grants guys. ust not at every stage of compaction. we also need to make expansion fast. why is it slow? the SCC part runs in a small number of seconds even for massive graphs.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants