Skip to content

sql performance improvements - #1737

Merged
nitisht merged 2 commits into
parseablehq:mainfrom
nikhilsinhaparseable:perf/sql
Jul 30, 2026
Merged

sql performance improvements#1737
nitisht merged 2 commits into
parseablehq:mainfrom
nikhilsinhaparseable:perf/sql

Conversation

@nikhilsinhaparseable

@nikhilsinhaparseable nikhilsinhaparseable commented Jul 29, 2026

Copy link
Copy Markdown
Member
  • add env P_PARQUET_METADATA_CACHE_SIZE for max parquet metadata cache size
    default 50 MB
  • update env P_DATAFUSION_TARGET_PARTITIONS default to 4*num_cpu
  • add metrics for parquet metadata cache reads

Summary by CodeRabbit

  • New Features

    • Added a CLI option to configure the Parquet metadata cache size, with support for environment-variable configuration.
    • Updated the default query partition setting to improve parallel query execution.
  • Bug Fixes

    • Removed inaccurate or unavailable Parquet metadata cache statistics from query reporting.
    • Retained existing bytes-scanned billing metrics after query completion.

1. add env `P_PARQUET_METADATA_CACHE_SIZE` for max parquet metadata cache size
default 50 MB

2. update env `P_DATAFUSION_TARGET_PARTITIONS` default to 4*num_cpu

3. add metrics for parquet metadata cache reads
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 75aa713f-6a16-42f2-82cf-255e75720c7f

📥 Commits

Reviewing files that changed from the base of the PR and between 8d453ee and d4c0401.

📒 Files selected for processing (1)
  • src/query/mod.rs

Walkthrough

The CLI adds a configurable Parquet metadata cache size, changes the default target partition calculation, applies the cache limit to DataFusion sessions, and removes Parquet metadata cache statistics reporting from query completion paths.

Changes

Parquet metadata cache configuration

Layer / File(s) Summary
CLI cache configuration
src/cli.rs
Adds parquet_metadata_cache_size with an environment variable and default, and changes the target_partitions default calculation.
Query cache configuration and reporting removal
src/query/mod.rs
Applies the configured metadata cache limit and removes cache statistics collection and reporting from non-streaming and final streaming execution paths.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: parmesant

Poem

I’m a bunny tuning caches with care,
Four times the partitions hop through the air.
Metadata limits now neatly align,
Old cache reports vanish from the line.
Hop, hop—queries finish fine!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description captures the changes but misses the template sections, issue reference, rationale, and checklist items. Add the Fixes #XXXX line, a Description section with goal/rationale, and the testing/docs checklist from the template.
Title check ❓ Inconclusive The title is related to the change but too generic to clearly convey the main update. Use a specific title such as "Add parquet metadata cache sizing and update DataFusion partitions".
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/query/mod.rs`:
- Around line 1073-1098: Move the full-cache aggregation currently performed
after query completion into a periodic snapshot refresh or
cache-operation/scrape-time metrics path. Remove the per-query enumeration and
sorting around entries, entry_sizes, percentile, and the
p50_entry_bytes/p95_entry_bytes/max_entry_bytes calculations, while preserving
the resulting cache metrics through the selected refresh mechanism.
- Around line 1111-1121: Change the parquet metadata cache stats log in the
query flow from tracing::warn! to tracing::debug! so successful queries do not
emit warning-level snapshots, while preserving all existing fields and message
content.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4dae90c5-eaeb-4888-80ff-961c3b5c6ab8

📥 Commits

Reviewing files that changed from the base of the PR and between 3901dfa and 8d453ee.

📒 Files selected for processing (3)
  • src/cli.rs
  • src/metrics/mod.rs
  • src/query/mod.rs

Comment thread src/query/mod.rs Outdated
Comment thread src/query/mod.rs Outdated
@coderabbitai coderabbitai Bot mentioned this pull request Jul 29, 2026
3 tasks
@nitisht
nitisht merged commit 0c4ce73 into parseablehq:main Jul 30, 2026
12 checks passed
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.

2 participants