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,
	),