perf(cubestore): turn on the performance toggles by default

[waralexrom]

Summary

Flips the defaults of the CubeStore performance/ingestion env toggles that have been opt-in so far, so a node gets them without extra configuration. Every toggle keeps its env var, so any of them can still be turned back off individually.

Changes

New defaults in Config::default_values():

Env	Was	Now
CUBESTORE_LOAD_AWARE_IMPORT_PLACEMENT	false	true
CUBESTORE_REPARTITION_STRATEGY	per_chunk	range
CUBESTORE_REPARTITION_CONCURRENT_DOWNLOAD	false	true
CUBESTORE_REPARTITION_MERGE_MAX_ROWS	4_000_000	400_000
CUBESTORE_CSV_IMPORT_JOB_RUNNERS	0	1
CUBESTORE_METASTORE_BATCH_RPC	false	true
CUBESTORE_GROUP_BY_LIMIT_FACTOR	0	2
CUBESTORE_GROUP_BY_LIMIT_PER_PARTITION	false	true
CUBESTORE_TOPK_STRATEGY	streaming	full_merge

CUBESTORE_COMPACTION_CHUNKS_THRESHOLD_MULTIPLIER was already 1.0 and is unchanged.

Supporting bits, kept to the minimum the flip requires:

• env_topk_strategy and env_repartition_strategy hold their default inside the parser, so their unset/unparseable fallbacks (and warning texts) move to FullMerge / Range. The "" and default aliases of CUBESTORE_TOPK_STRATEGY follow the new default instead of resolving to streaming.
• env_flag takes the default as an argument — GROUP_BY_LIMIT_PER_PARTITION needs true, while it previously hardcoded false. Parsing stays lenient (1/true enable, anything else is off, never panics); CUBESTORE_COALESCE_UNDER_HASH_AGGREGATE passes false explicitly and is unaffected.
• The (default) marker in the TopKAggregateStrategy docs moves from Streaming to FullMerge.

Config::test(...) is untouched: it sets these fields explicitly, so the test suite keeps running the previous modes, and the tests that cover the newly-default paths keep opting into them by hand.

Testing

• cargo check -p cubestore --lib
• cargo fmt --all -- --check

[claude]

Claude finished @waralexrom's task in 2m 48s —— View job

---

Review complete — mechanically correct, but 1 CI blocker and 4 stale-doc/coverage gaps (1 high, 2 medium, 2 low)

Full review — expand for details

• Gather context (diff, config file, flag call sites)
• Review defaults flip & parser changes
• Check test/docs impact
• Post review

Summary

The flip itself is done consistently: every toggle keeps its env var, the two lenient parsers (env_topk_strategy, env_repartition_strategy) move their unset/unparseable fallback and their warning text together with the default, and env_flag gaining a default parameter is the right minimal change (both call sites pass it explicitly). No panic paths were introduced. Nice touch keeping the "" | "default" aliases pointing at the new default rather than pinning them to streaming.

1. cargo fmt will fail (high) — inline comment

rust/cubestore/cubestore/src/config/mod.rs:1898-1899 are 103 and 106 columns. rustfmt's 100-column max_width applies to these call expressions (unlike the other long lines in the file, which are string literals/comments), so cargo fmt --all -- --check fails. Suggestion posted inline.

2. Stale ConfigObj doc comments (medium)

The trait docs still describe the old defaults, and they're the primary in-repo documentation for these flags:

Line	Current text	Now
config/mod.rs:537-538	"Off by default (hash placement)"	on by default
config/mod.rs:566	"Off by default." (repartition_concurrent_download)	on by default
config/mod.rs:570	"Defaults to PerChunk."	defaults to Range
config/mod.rs:592	"false (default) coalesces the partial aggregate's input…"	true is the default
config/mod.rs:586-588	group_by_limit_factor — "0 disables the optimization" is still accurate, but it no longer says the default is now 2 (on)	worth stating

Also outside the diff: queryplanner/optimizations/distributed_partial_aggregate.rs:444 says // - off (default): coalesce the aggregate's input to a single partition, which is now inverted. That comment is the clearest explanation of the memory trade-off in the codebase, so it's the one most likely to mislead. Fix this →

3. The shipped default config is now the least-tested one (medium)

Config::test(...) is deliberately left alone, so the suite keeps running PerChunk, TopKAggregateStrategy::Streaming, group_by_limit_per_partition: false, repartition_concurrent_download: false, metastore_batch_rpc: false, repartition_merge_max_rows: 4_000_000. That's a defensible way to keep the diff small, but the consequence is that after this PR every default-config code path is exercised only by the handful of tests that opt in by hand (sql/mod.rs:3986/4097/4115, store/mod.rs:1624/2062, compaction.rs:1864), while the paths nobody runs in production remain the ones with blanket coverage. prefilter_in_memory_chunks_enabled in Config::test already sets the precedent for the opposite choice — it's forced on in tests with a comment explaining why. Flipping the test config to match production (and pinning the few tests that specifically assert old-mode behavior) would be the higher-confidence route; if you'd rather not do it in this PR, a comment in Config::test recording that these now intentionally diverge from prod would at least make the gap visible.

4. A typo in a now-default-on flag silently disables it (low)

env_flag treats anything other than 1/true as off, with no warning. That was harmless while the default was false (a typo was a no-op), but with CUBESTORE_GROUP_BY_LIMIT_PER_PARTITION defaulting to true, TRUE, True, yes, or on now silently turn off a behavior the operator was trying to confirm. Both sibling parsers in this file (env_topk_strategy, env_repartition_strategy) log::warn! on an unrecognized value — env_flag could do the same, or at least accept case-insensitive true/1.

5. Rollout notes worth calling out in the description (low)

Not defects, but the combination changes the memory and job-churn profile of a default node, and these are the things an operator would want in the release note:

• topk_aggregate_strategy: FullMerge drops early termination so the router materializes every distinct group, while group_by_limit_per_partition: true moves the worker to N parallel hash tables (peak ~N*k per the comment at distributed_partial_aggregate.rs:446-450). Both raise peak memory, on different tiers, at the same time.
• repartition_merge_max_rows: 4_000_000 → 400_000 is a 10× cut, so a repartition of the same parent produces ~10× the merge groups — more jobs, more parquet writes, more metastore traffic per repartition.
• csv_import_job_runners_count: 0 → 1 adds a permanent job-runner loop on every node (cluster/mod.rs:1332), which also means CSV-import jobs now get a dedicated runner instead of never being drained from that pool.
• metastore_batch_rpc: true makes workers issue batched metastore calls (e.g. get_active_partitions_for_indexes) by default. Worth confirming the behavior in a mixed-version cluster during a rolling upgrade, where a new worker may be talking to a router that predates those RPC methods.

No prior review threads existed on this PR, so nothing to resolve or dedupe against.

• [branch `cubestore-change-perf-defaults`](https://github.com/cube-js/cube/tree/cubestore-change-perf-defaults)

[claude]

Both of these lines are 103 and 106 columns wide, so cargo fmt --all -- --check fails here (rustfmt's 100-column max_width applies to these call expressions; the other long lines in this file are string literals/comments, which rustfmt leaves alone). The PR description says fmt was clean — worth re-running, CI will flag it.

Suggested change

	group_by_limit_per_partition: env_flag("CUBESTORE_GROUP_BY_LIMIT_PER_PARTITION", true),
	coalesce_under_hash_aggregate: env_flag("CUBESTORE_COALESCE_UNDER_HASH_AGGREGATE", false),
	group_by_limit_per_partition: env_flag(
	"CUBESTORE_GROUP_BY_LIMIT_PER_PARTITION",
	true,
	),
	coalesce_under_hash_aggregate: env_flag(
	"CUBESTORE_COALESCE_UNDER_HASH_AGGREGATE",
	false,
	),

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @waralexrom's task in 4m 40s —— View job

---

Re-review of the 2 new commits — docs & test-config feedback addressed; 1 new high (rolling-upgrade hazard), 1 CI blocker still open, 2 medium/low

Full review — expand for details

• Gather context (diff since last review, existing threads)
• Re-check the 5 findings from the previous review
• Review the new test-config + docs commits
• Post review

What the new commits fixed

6ca6991 and 539b6d2 cleanly close three of the five items from the last pass:

• Stale doc comments (was MUTEX for requests #2) — all five ConfigObj docs now state the new defaults, and the distributed_partial_aggregate.rs:443-450 bullet list is re-ordered so on (default) leads. The TopKAggregateStrategy variant docs moved the (default) marker too. Nothing stale left that I can find.
• Test config diverging from prod (was ResultSet pivot methods documentation  #3) — Config::test now runs the production defaults, and the tests that assert old-mode behavior were pinned by hand (scheduler/mod.rs:1787 sets load_aware_import_placement_enabled = false and the test was renamed off _by_default). This is the higher-confidence route, and the plan-expectation churn in tests.rs is the honest cost of it.
• Rollout notes (was Add support for AWS Athena in JDBC driver #5) — the mixed-cluster hazard is now written down at config/mod.rs:1308-1313. See below though: writing it down raised a question rather than answering it.

1. The flip turns a configuration hazard into an upgrade hazard (high) — inline comment

The new doc comment states that a cluster running a mix of top-k strategies "returns wrong top-k rows instead of failing". I confirmed the mechanism — router_context and worker_context (queryplanner/query_executor.rs:570-591) each read topk_aggregate_strategy() / group_by_limit_factor() from their own process, and the strategy is not carried in PreSerializedPlan.

While the default was streaming, a mix required someone to set the env var unevenly. After this PR it happens on every rolling upgrade of a multi-node cluster with no env var set at all: in the window where workers are new and the router is old, the router plans AggregateTopKExec (NRA merge, assumes sorted worker streams, early-terminates) over workers now sending unsorted full-merge output → silently wrong rows.

Best fix is to plumb the strategy through the serialized plan so the router's choice is authoritative; short of that, this needs an operator-facing upgrade note (pin CUBESTORE_TOPK_STRATEGY=streaming cluster-wide before the rollout, unpin after), not just a Rust doc comment.

2. cargo fmt still fails (high) — existing thread, unresolved

config/mod.rs:1905 and :1906 are still 103 and 106 columns. Re-affirming the prior thread rather than opening a new one; the suggestion there still applies as a drop-in.

3. The non-default paths now have no coverage at all (medium)

Flipping Config::test was the right call, but it moved the coverage rather than widening it. After this PR nothing in the suite sets:

Flag	Uncovered value
group_by_limit_per_partition	false (the "over merge" single-hash-table path)
repartition_concurrent_download	false (sequential download)
metastore_batch_rpc	false (unbatched RPC)
topk_aggregate_strategy	Streaming / VectorizedStreaming for non-HLL queries

All four remain supported and env-selectable — they're the documented rollback lever for this PR, so they're the paths an operator reaches for precisely when something is wrong, and they're now the ones nobody runs. (The streaming merge itself keeps coverage: planning_topk_having's HLL query and planning_topk_hll still route through AggregateTopKExec, plus the unit tests at topk/execute.rs:1568+.) Pinning one existing test per row to the old value would restore both sides cheaply.

Related tidy-up: several explicit opt-ins are now redundant no-ops — sql/mod.rs:3486 (FullMerge), sql/mod.rs:4058 (repartition_concurrent_download = true), store/mod.rs:1624 and :2062 (metastore_batch_rpc = true). Flipping those to the old value instead of deleting them would fix this finding and the redundancy in one move. Fix this →

4. planning_topk_having skipped on the cluster config (low) — inline comment

Removes the only 2-node assertion of the newly-default router plan shape. Execution coverage survives via topk_having / topk_query, so not a correctness gap — but a cluster-specific expected plan beats a skip, since the plan shape is the thing that differs.

5. env_flag still silently disables a now-default-on toggle (low)

Unchanged from the last pass and still worth a line: TRUE, True, yes, on all resolve to false with no warning, which now disables CUBESTORE_GROUP_BY_LIMIT_PER_PARTITION for an operator who was trying to confirm it. Both sibling parsers in the file log::warn! on unrecognized input.

Notes

• Could not run cargo fmt --all -- --check or the test suite in this environment (network fetch and cargo invocations are not in the allowed tool set), so findings MUTEX for requests #2 and ResultSet pivot methods documentation  #3 are from reading the source; the column counts in MUTEX for requests #2 are measured directly off the file.
• Re-affirmed 1 prior thread (MUTEX for requests #2); did not resolve it, as it still applies.

• [branch `cubestore-change-perf-defaults`](https://github.com/cube-js/cube/tree/cubestore-change-perf-defaults)

[claude]

This comment is the most important thing in the PR, and it argues against shipping the flip unguarded.

Confirmed the mechanism: the router plans its half in router_context and the worker re-plans its half in worker_context (queryplanner/query_executor.rs:570-591), each reading self.config.topk_aggregate_strategy() / group_by_limit_factor() from its own process env. The strategy is not carried in PreSerializedPlan, so nothing ties the two halves together.

While the default was streaming, a mixed cluster could only happen if an operator set the env var on some nodes and not others. After this PR it happens on every rolling upgrade of a multi-node cluster, without anyone touching a single env var — during the window where workers are on the new build and the router is still on the old one, the router plans AggregateTopKExec (NRA merge, which assumes each worker stream arrives sorted by the order-by aggregate and early-terminates) over workers that are now sending unsorted full-merge output. Per this very comment, that returns wrong rows rather than erroring.

Two things would close it, and I'd want at least the first:

1. Plumb the strategy (and group_by_limit_factor/group_by_limit_per_partition) through the serialized plan so the router's choice is authoritative and the worker plans the matching half. That makes the config a cluster-level decision by construction and removes the ordering constraint on the upgrade entirely.
2. Failing that, an upgrade note: operators must pin CUBESTORE_TOPK_STRATEGY=streaming cluster-wide before upgrading, then flip it after every node is on the new build — otherwise queries silently return wrong top-k during the rollout.

A doc comment on a Rust enum is not reachable by the person running the upgrade.

[claude]

Skipping planning_topk_having on the 2-node config removes the only assertion of the newly-default full-merge router plan shape under more than one worker — which is the case this PR makes universal. The single-node expectation added in tests.rs:4487 has the router running SortedSingleAggregate over CoalescePartitions; with one ClusterSend partition that's fine, but SortedSingleAggregate over a coalesce of N worker streams would emit duplicate group rows, and the cluster config is exactly where that shows up.

Execution coverage does survive (topk_having and topk_query still run on the cluster config with real rows, so a duplicate-groups regression would be caught), so this is not a correctness gap today — but a cluster-specific expected plan would be more useful than a skip, since the plan shape is the thing that differs and the thing nobody is now watching.