Skip to content

Conversation

@wjones127
Copy link
Contributor

@wjones127 wjones127 commented Jan 16, 2026

Motivation

We have a performance issue when LANCE_MEM_POOL_SIZE: when this is set (to something different than the default), the current cache always misses here:

pub fn get_session_context(options: &LanceExecutionOptions) -> SessionContext {
if options.mem_pool_size() == DEFAULT_LANCE_MEM_POOL_SIZE
&& options.max_temp_directory_size() == DEFAULT_LANCE_MAX_TEMP_DIRECTORY_SIZE
&& options.target_partition.is_none()
{
return if options.use_spilling() {
DEFAULT_SESSION_CONTEXT_WITH_SPILLING.clone()
} else {
DEFAULT_SESSION_CONTEXT.clone()
};
}
new_session_context(options)
}

Summary

  • Replace static LazyLock session contexts with LRU cache (size 4)
  • Cache key uses resolved configuration values (after env var lookup)
  • Fixes cache misses when LANCE_MEM_POOL_SIZE env var is set

Test plan

  • cargo test -p lance-datafusion passes
  • cargo clippy -p lance-datafusion passes

🤖 Generated with Claude Code

Previously, get_session_context only cached session contexts when options
matched hardcoded defaults. This meant setting LANCE_MEM_POOL_SIZE env var
would cause cache misses every time, creating a new SessionContext on each
call.

Now uses an LRU cache (size 4) keyed by resolved configuration values,
so repeated calls with the same effective configuration reuse cached
contexts regardless of whether values come from defaults, explicit
options, or environment variables.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

Code Review

P1: Missing test coverage for new caching logic

The new LRU cache implementation has no unit tests. Given this code manages shared state (global cache with mutex), I'd recommend adding tests to verify:

  • Cache hits return the same context
  • LRU eviction works correctly when cache exceeds MAX_CACHE_SIZE
  • Different options produce different cache keys

P1: Consider using lru crate instead of manual implementation

The manual LRU implementation using HashMap + Instant requires iterating all entries to find the LRU item on eviction (O(n) instead of O(1)). While n=4 is small, using the lru crate would be cleaner and avoid potential bugs. The current implementation also holds the mutex lock during the O(n) eviction scan.

Minor observation

The change looks functionally correct and solves the stated problem (env var values not being considered for caching). The cache key using resolved values is the right approach.

@wjones127 wjones127 changed the title refactor: use LRU cache for session contexts in get_session_context perf: use LRU cache for session contexts in get_session_context Jan 16, 2026
Tests verify:
- Caching works (same options reuse cached context)
- Different options create separate cache entries
- LRU eviction order (least recently accessed is evicted first)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@wjones127 wjones127 force-pushed the refactor/session-context-lru-cache branch from 7ca8443 to 822c5f1 Compare January 16, 2026 21:54
@codecov
Copy link

codecov bot commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 96.87500% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-datafusion/src/exec.rs 96.87% 0 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

Use unwrap_or_else with into_inner() to recover from PoisonError
instead of panicking. The cache data remains valid even if a thread
panicked while holding the lock.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@wjones127 wjones127 marked this pull request as ready for review January 16, 2026 23:08
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@wjones127 wjones127 requested a review from cmccabe January 17, 2026 00:26
Tests share global cache state and were failing when run in parallel.
Add a test-only Mutex to ensure they run sequentially.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@wjones127 wjones127 merged commit 1dcc562 into lance-format:main Jan 17, 2026
28 checks passed
jackye1995 pushed a commit to jackye1995/lance that referenced this pull request Jan 20, 2026
…e-format#5736)

We have a performance issue when `LANCE_MEM_POOL_SIZE`: when this is set
(to something different than the default), the current cache always
misses here:

https://github.com/lance-format/lance/blob/445dd5bfe7b0111fd8e146163393e825e5995679/rust/lance-datafusion/src/exec.rs#L398-L410

- Replace static `LazyLock` session contexts with LRU cache (size 4)
- Cache key uses resolved configuration values (after env var lookup)
- Fixes cache misses when `LANCE_MEM_POOL_SIZE` env var is set

- [x] `cargo test -p lance-datafusion` passes
- [x] `cargo clippy -p lance-datafusion` passes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
@jackye1995 jackye1995 mentioned this pull request Jan 20, 2026
jackye1995 pushed a commit to jackye1995/lance that referenced this pull request Jan 21, 2026
…e-format#5736)

## Motivation

We have a performance issue when `LANCE_MEM_POOL_SIZE`: when this is set
(to something different than the default), the current cache always
misses here:


https://github.com/lance-format/lance/blob/445dd5bfe7b0111fd8e146163393e825e5995679/rust/lance-datafusion/src/exec.rs#L398-L410

## Summary
- Replace static `LazyLock` session contexts with LRU cache (size 4)
- Cache key uses resolved configuration values (after env var lookup)
- Fixes cache misses when `LANCE_MEM_POOL_SIZE` env var is set

## Test plan
- [x] `cargo test -p lance-datafusion` passes
- [x] `cargo clippy -p lance-datafusion` passes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
jackye1995 pushed a commit that referenced this pull request Jan 21, 2026
We have a performance issue when `LANCE_MEM_POOL_SIZE`: when this is set
(to something different than the default), the current cache always
misses here:

https://github.com/lance-format/lance/blob/445dd5bfe7b0111fd8e146163393e825e5995679/rust/lance-datafusion/src/exec.rs#L398-L410

- Replace static `LazyLock` session contexts with LRU cache (size 4)
- Cache key uses resolved configuration values (after env var lookup)
- Fixes cache misses when `LANCE_MEM_POOL_SIZE` env var is set

- [x] `cargo test -p lance-datafusion` passes
- [x] `cargo clippy -p lance-datafusion` passes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants