Skip to content

GH-51367: [C++] Remove unused arrow::internal::LruCache and its tests/benchmarks - #51384

Open
basantjamwal wants to merge 1 commit into
apache:mainfrom
basantjamwal:gh-51367-remove-unused-lrucache
Open

basantjamwal wants to merge 1 commit into
apache:mainfrom
basantjamwal:gh-51367-remove-unused-lrucache

Conversation

@basantjamwal

Copy link
Copy Markdown

Closes #51367

Rationale for this change

No code within Arrow uses arrow::internal::LruCache. Per discussion in
#51328 (review comment r4027861697), removing this dead code along with
its tests and benchmarks.

What changes are included in this PR?

  • Removed cpp/src/arrow/util/cache_internal.h (LruCache, MemoizeLru,
    MemoizeLruThreadUnsafe — all unused outside this file and its tests)
  • Removed cpp/src/arrow/util/cache_test.cc
  • Removed cpp/src/arrow/util/cache_benchmark.cc
  • Removed corresponding entries from cpp/src/arrow/util/CMakeLists.txt

Verified via git grep across cpp/ that nothing else references this
code (gandiva's separate LRU cache in cpp/src/gandiva/lru_cache.h is
unrelated and untouched).

Supersedes #51371, which was opened with an empty diff due to a branch mix-up.

Are these changes tested?

Pure removal of unused code; no new tests needed. Relying on CI to
validate the build/test suite still passes.

Are there any user-facing changes?

No.

Was AI used for this PR?

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format.

If this is not a minor PR, could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

See also:

@github-actions
github-actions Bot marked this pull request as draft September 17, 2026 19:05
@basantjamwal
basantjamwal marked this pull request as ready for review September 17, 2026 19:06
@taepper

taepper commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

For ease of reference, this is the commit that originally introduced the LruCache:
5647e90#diff-884a1f1048151e271ec0858df8ff575da3b109b1302d8259bcedeaee93495685

Why was it introduced? I found this discussion and this dependent issue but no further references to the LruCache

@pitrou

pitrou commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

Caching some expensive compute kernel precomputations (such as compiling a regex) was the original intent but I then noticed two problems:

  1. The current LRU cache implementation is not extremely fast (partly because of the hash table I assume), which implies it may be detrimental in some cases (for example a very simple regex)
  2. How to cache such precomputed kernel state is not obvious in the current architecture (it's not clear whether KernelState is meant to be invocation-specific or should be reusable)

@taepper taepper left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty much a revert of 5647e90, so it looks good to me!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] Remove unused arrow::internal::LruCache and the corresponding tests and benchmarks

3 participants