[codex] Harden source multi-key pushdown reducers#59
Conversation
Classify multi-key source aggregates separately and route JSON aggregate pushdown through a shared typed reducer with encoded-key lookup. Keep CSV and line multi-key pushdown on their existing paths after local benchmarks showed the shared reducer was slower there. Extend the benchmark gate to require the JSON typed path to beat generic execution.
Keep JSON multi-key aggregation on semantic-safe encoded lookup with a single map probe and cover numeric key semantics for typed and generic reducers. Add CSV and line selected-vs-generic benchmark guardrails, short-circuit empty compiled predicates, and document the rejected packed-key candidate.
ashione
force-pushed
the
auto/source-typed-reducer
branch
from
3d439b8 to
e148c6a
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e148c6ab13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| aggregate_value = | ||
| parsedScalarToValue(row_values[static_cast<std::size_t>( | ||
| access_plan.local_value_by_aggregate.front())]); | ||
| aggregate_value_ptr = &aggregate_value; |
There was a problem hiding this comment.
In the JSON multi-key typed path, parsedScalarToValue re-parses quoted JSON string scalars with parseScalarCell, so a value like "10" becomes numeric and contributes to SUM/AVG. The generic JSON aggregate path uses updateAggregateStatesParsed, which only sums is_int64/is_double tokens, matching load_json_file where quoted numbers remain strings. This makes selected vs Generic pushdown return different results for multi-key JSON SUM/AVG when the aggregate column contains quoted numeric strings.
Useful? React with 👍 / 👎.
| cache->schema = table.schema; | ||
| cache->columns.resize(table.schema.fields.size()); | ||
| table.columnar_cache = std::move(cache); |
There was a problem hiding this comment.
When the JSON multi-key path returns typed_reducer.finalize(), this helper builds a columnar cache with columns sized but leaves arrow_formats empty. The repo's columnar invariant requires arrow_formats.size() == columns.size() in validateColumnarCache, so downstream validation or append-column paths see every typed aggregate result as an invalid columnar cache. Resize arrow_formats alongside columns here.
Useful? React with 👍 / 👎.
1 participant
Summary
COUNT, numericSUM, andAVGbehind the existing source pushdown contract.Valuekeys for numeric, bool, and null cases.Validation
bazel test //:core_regression --test_output=errorsbazel build //:sql_demo //:df_demo //:stream_demo./scripts/run_columnar_kernel_benchmark_gate.shgit diff --checkNotes
The retained benchmark-backed win is JSON multi-key typed aggregate pushdown. CSV and line multi-key shared reducer attempts were practiced and rejected because repeated local benchmark runs did not prove stable wins; the benchmark gate now checks those paths against generic execution.