Skip to content

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

Open
basantjamwal wants to merge 3 commits into
apache:mainfrom
basantjamwal:gh-51367-remove-unused-lrucache
Open

basantjamwal wants to merge 3 commits 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!

@basantjamwal

Copy link
Copy Markdown
Author

Hi @pitrou @taepper,

The only failing check is "AMD64 Windows 2022 MATLAB". The build and all 1,475 tests pass. It fails in the "Post Cache ccache" step (hashFiles('cpp/**', 'matlab/**')), which matches the known flake in #51335.

This PR only deletes unused C++ files and two CMake entries, so it shouldn't be related.

Could one of you re-run that job, or merge if you're comfortable with it? Thanks!

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 22, 2026
@dmitry-chirkov-dremio

Copy link
Copy Markdown
Contributor

My agent says "CI: Extra" or "CI: Extra: C++" labels would bring out Meson build failure:

The CMake entries are removed, but the Meson build still lists cache_test.cc and the cache benchmark, which expands to cache_benchmark.cc. Both files are deleted here, so Meson builds with tests or benchmarks enabled will fail. Please remove these two Meson entries as well.

Comment thread cpp/src/arrow/util/CMakeLists.txt Resolved

@HuaHuaY HuaHuaY 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.

My agent says "CI: Extra" or "CI: Extra: C++" labels would bring out Meson build failure:

Good catch. We need to modify cpp/src/arrow/util/meson.build.

@basantjamwal

Copy link
Copy Markdown
Author

Hi @pitrou , can you please review my PR? Thank you!

@pitrou pitrou added the CI: Extra: C++ Run extra C++ CI label Nov 9, 2026

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, let's wait for CI

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

Labels

awaiting committer review Awaiting committer review CI: Extra: C++ Run extra C++ CI Component: C++

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

5 participants