[Analytics] Add optional liquid cache seam for indexed parquet scans (cargo feature)

[cocosz]

Description

Adds an opt-in, in-memory decoded-batch cache ("liquid cache") for the DataFusion analytics backend, wired behind a cargo feature so it is completely absent from the default build.

• Feature off (default): the integration is an inlined no-op. Eligible scans use the plain ParquetSource unchanged, and the build declares no liquid cache dependency, so it resolves and compiles exactly as before.
• Feature on (-PliquidCache / LIQUID_CACHE=1): an eligible indexed row-group scan is wrapped with LiquidParquetSource, so decoded batches are served from and populated into the process-global cache.

The hook (liquid_cache.rs) is already in its final form: when the feature is on it delegates to the opensearch-liquid-cache crate. That crate is provided by the liquid-cache plugin later in the stack, so here the dependency is left commented in Cargo.toml (Cargo resolves every declared path dependency even when its feature is off, which would otherwise break this build). The integration PR only flips the dependency on — it does not modify this hook.

This is the first PR in a 3-part stack that keeps each change small and independently reviewable. The stack is a straight split of the final code, not an evolution — each file lands in its final shape:

1. This PR — seam: the feature-gated in-process hook (no-op by default).
2. Library ([Analytics] Vendor the liquid-cache library crates #22569): vendors the liquid-cache library crates in isolation.
3. Integration ([Analytics] Wire liquid cache into the analytics engine (plugin + integration) #22595): turns the dependency on, adds the plugin + Java wiring + integration test.

Changes

File	What
sandbox/libs/dataformat-native/build.gradle	-PliquidCache / LIQUID_CACHE=1 build flag that forwards the cargo feature
.../analytics-backend-datafusion/rust/Cargo.toml	commented optional opensearch-liquid-cache dep + bare liquid_cache = [] feature (default build resolves no dep)
.../rust/src/indexed_table/parquet_bridge.rs	call site for the maybe_wrap_parquet_source hook (no-op when feature off)
.../rust/src/lib.rs	pub mod liquid_cache
.../rust/src/liquid_cache.rs	the hook: no-op when off; when on, delegates to opensearch-liquid-cache to wrap eligible scans

5 files changed, +95 / -3.

Check List

• Default build compiles with the feature off (no behavior change, no new dependency).
• Functionality verified with the feature on (cache engages on eligible indexed scans) — via the integration PR, which supplies the crate.
• API changes companion PR raised (n/a).
• Public documentation issue/PR created (n/a — sandbox).

Note: PRs target main, so until the parent merges the GitHub diff for the downstream PRs includes their parent commits. Review/merge in stack order: this PR → #22569 → #22595.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit a09b338.

⛔ Hard block: Issues at Medium severity or above will block this PR from merging.

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-datafusion/rust/Cargo.toml	99	high	New dependency 'opensearch-liquid-cache' introduced in Cargo.toml (currently commented out, pointing to a local path '../../liquid-cache/src/main/rust'). Per mandatory flagging rule, all dependency additions must be flagged regardless of whether they appear active or commented. Maintainers should verify the local crate at that path is legitimate and not a placeholder for a future registry dependency swap.
sandbox/libs/dataformat-native/build.gradle	80	low	Build feature activation is gated on the environment variable LIQUID_CACHE=1. If an attacker can inject environment variables into CI/CD pipelines (e.g., via a misconfigured secrets store or build environment), they could silently enable the liquid_cache Cargo feature, which activates the commented-out crate and wraps all Parquet data reads through a process-global cache layer. The risk is low given the feature is off by default and the dep is commented, but the env-var trigger is worth noting.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[alchemist51]

Can we remove the plugin and verify if the changes are still compiling? The whole idea is to move the liquid cache to a separate plugin without taking the direct dependency here.

[alchemist51]

This actually creates the dependency on the liquid cache. How will this work without the liquid cache plugin?

[cocosz]

Addressed. Liquid cache is now an optional, off-by-default cargo dependency of the native lib, so the default build takes no dependency on it and the execution backend never names it.

[alchemist51]

We need a config option which should enable this, it should not be default be present.

[cocosz]

Done — it's now optional behind a cargo feature that's off by default.

[alchemist51]

liquid cache is still here?

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 600387b)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Feature Wired to Commented Dependency The liquid_cache feature is declared as an empty list with the actual dep:opensearch-liquid-cache entry commented out. Building with --features liquid_cache will enable the #[cfg(feature = "liquid_cache")] block in liquid_cache.rs, which imports from opensearch_liquid_cache, but that crate is not declared as a dependency. This will cause a compile failure until the integration PR uncomments the dep. Consider gating the feature more defensively or documenting that enabling the feature in this PR alone will not compile. # Liquid Cache (off by default): uncomment the dep + the dep: entry below and build with -PliquidCache to use liquid cache. # opensearch-liquid-cache = { path = "../../liquid-cache/src/main/rust", optional = true } [features] # Off by default; when on, the hook in src/liquid_cache.rs engages the process-global cache. liquid_cache = [] # ["dep:opensearch-liquid-cache"]

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 600387b
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Feature flag references missing optional dependency Building with --features liquid_cache will fail to compile because src/liquid_cache.rs references opensearch_liquid_cache::* under #[cfg(feature = "liquid_cache")], but the dependency is commented out and the feature array is empty. Either enable the dependency or make the feature include it so the gated code path builds. sandbox/plugins/analytics-backend-datafusion/rust/Cargo.toml [98-103] +opensearch-liquid-cache = { path = "../../liquid-cache/src/main/rust", optional = true } + [features] # Off by default; when on, the hook in src/liquid_cache.rs engages the process-global cache. -liquid_cache = [] # ["dep:opensearch-liquid-cache"] +liquid_cache = ["dep:opensearch-liquid-cache"] Suggestion importance[1-10]: 6 __ Why: Valid observation: building with --features liquid_cache would fail since the dependency is commented out. However, the PR intentionally leaves this commented out as a manual opt-in step (per the comment), so the impact is moderate.	Low
General	Scope the unused-variable suppression correctly The #[cfg(not(feature = "liquid_cache"))] let _ = config; only silences the unused warning for config, but when the feature is enabled and none of the inner branches are taken, config is still unused on that path only in some builds. More importantly, mark the parameter with _ prefix or use #[allow(unused_variables)] on the function to avoid inconsistent lint behavior across feature combinations. sandbox/plugins/analytics-backend-datafusion/rust/src/liquid_cache.rs [40-43] #[cfg(not(feature = "liquid_cache"))] -let _ = config; +{ + let _ = config; +} Arc::new(parquet_source) Suggestion importance[1-10]: 2 __ Why: The suggestion is minor and largely stylistic; the existing let _ = config; already suppresses the unused warning correctly, and the proposed change wrapping it in a block offers negligible improvement.	Low

---

Previous suggestions

Suggestions up to commit b4e7524

Category	Suggestion	Impact
Possible issue	Resolve predicate columns by name, not index The collect_columns helper returns physical Column expressions whose index() refers to the projected schema index, not the full-schema index. Looking those up in config.full_schema.fields() will index the wrong columns (or panic-free but incorrect) and can miss string predicates. Resolve columns by name against full_schema, or index the projected schema instead. sandbox/plugins/analytics-backend-datafusion/rust/src/liquid_cache.rs [105-121] -if all_numeric_projection == false { +if !all_numeric_projection { return false; } let predicate_has_string = config.predicate.as_ref().is_some_and(|pred| { datafusion::physical_expr::utils::collect_columns(pred) .iter() .any(|col| { config .full_schema - .fields() - .get(col.index()) - .is_some_and(|f| is_string_or_binary(f.data_type())) + .field_with_name(col.name()) + .map(|f| is_string_or_binary(f.data_type())) + .unwrap_or(false) }) }); -predicate_has_string == false +!predicate_has_string Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential correctness issue: collect_columns returns physical Column expressions whose index refers to the schema the predicate was constructed against (often the projected/filter schema, not the full schema). Resolving by name against full_schema is safer and avoids missing string predicates or misclassifying them. This is a plausible bug though it depends on how the predicate is constructed upstream.	Medium

[github-actions]

Persistent review updated to latest commit 600387b