[pull] master from cube-js:master - #686
Merged
Merged
Conversation
#11631) * fix(cubestore): recompute inline aggregate plan properties on input swap InlineAggregateExec carried its PlanProperties over to a new child instead of recomputing them. The node is converted from an AggregateExec while a CoalescePartitionsExec sits under it, so the cached output partitioning is 1; EnforceSorting then strips that coalesce as an avoidable bottleneck and reattaches the multi-partition subtree. The stale count stays 1, the parent executes only partition 0, and every row in the remaining partitions is dropped with no error. Delegate the input swap to the AggregateExec the node was built from, then re-convert. DataFusion recomputes the properties there and preserves the output schema, which it derives from aggregate expression names its own rules may rewrite. When the new input is no longer sorted on the group keys the plain hash aggregate is kept, without the limit: on the streaming path it counts complete groups off a sorted input, while a hash aggregate reads it as a cap on distinct groups and would truncate arbitrary ones. Reachable only when every group-by column is pinned to a single value by the filter (which is what makes the aggregate sorted), the leading sort key column is outside the filter set so partition pruning cannot collapse the scan, and the matching rows live outside the first partition. A rollup partitioned by a time dimension that the query does not filter is exactly that shape. Also adds an exhaustive check that MinMaxCondition never prunes a range holding a matching row -- partition pruning was the first suspect and is clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(cubestore): derive group-by-limit aggregate properties from the new child GroupByLimitAggregateExec refreshed only the output partitioning when its input was swapped, leaving the rest of the plan properties -- orderings and constants projected from the previous input -- behind. Delegate the swap to the aggregate it was built from, the same way the inline aggregate does, so DataFusion recomputes all of them and preserves the output schema. If the rebuilt aggregate no longer fits the trimming path it is returned as is. The other two nodes that carry their properties over, RollingWindowAggExec and AggregateTopKExec, build them from their own output schema plus constants and read the input's partition count live, so nothing there can go stale; say so at both sites, since the same pattern loses rows in a node whose partitioning follows the input. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(cubestore): pin the aggregate node behind the partitioning fix Review follow-ups on the inline aggregate fix. The end-to-end regression test asserted only on rows, so a planner change that moved the query off the streaming aggregate, or collapsed the scan to one partition, would have kept it green while covering nothing. Assert the plan shape too, and add the unit-level counterpart the sibling node already has: build the exec over a one-partition input, re-child it onto three, and require both the reported partitioning and the node type to follow. It runs in milliseconds and does not depend on compaction timing. Both fallbacks -- to the hash aggregate here, out of the trimming path in GroupByLimitAggregateExec -- now name the group by and the new input's order mode, so a degradation is diagnosable from the log rather than showing up only as wrong-looking timings. The group-by-limit test also pins that re-childing keeps the trimming exec, which the new fallback would otherwise let it pass without. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
rust
data source driver
cube store
javascript
pr:community
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )