You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "naman-modi (via GitHub)" <gi...@apache.org> on 2026/07/23 14:00:00 UTC
[PR] test (slt): add memory-limited aggregation sqllogictests [datafusion]
naman-modi opened a new pull request, #23838:
URL: https://github.com/apache/datafusion/pull/23838
## Which issue does this PR close?
- Part of #22710.
## Rationale for this change
There's almost no slt coverage for grouped aggregation under a memory limit, where the aggregate spills to disk and re-groups the spilled state. #23657 covers the ordered path & this covers the unordered/hash path.
## What changes are included in this PR?
- Adds `aggregate_memory_limit.slt` in which the high-cardinality "GROUP BY" is run under a 1M limit, so the hash aggregate spills. The group key is scrambled with `(v * 7) % 100000` (a bijection, so still 100000 groups) to keep the input unsorted; otherwise it takes the streaming path and never spills.
- Scoped to a single partition (`target_partitions = 1`), so the spill happens in one aggregate operator with no repartition.
- Cases cover different accumulator states: single-column, multi-column, count(DISTINCT), sum/min/max, avg (widening), array_agg (growable).
## Are these changes tested?
This PR is tests. All pass locally.
## Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test (slt): add memory-limited aggregation sqllogictests [datafusion]
Posted by "naman-modi (via GitHub)" <gi...@apache.org>.
naman-modi commented on PR #23838:
URL: https://github.com/apache/datafusion/pull/23838#issuecomment-5059332883
Hey @2010YOUY01, can you please review this, whenever you get a chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test (slt): add memory-limited aggregation sqllogictests [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #23838:
URL: https://github.com/apache/datafusion/pull/23838#discussion_r3643705212
##########
datafusion/sqllogictest/test_files/aggregate_memory_limit.slt:
##########
@@ -0,0 +1,137 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Memory-limited (spilling) grouped hash aggregation.
+#
+# High-cardinality GROUP BY under a tight memory limit: the aggregate spills to
+# disk, re-groups the spilled state, and must still return the right answer.
+#
+# The group key is scrambled with `(v * 7) % 100000` because generate_series is
+# sorted, which would take the streaming path that never spills. gcd(7, 100000)
+# = 1, so it's a bijection over 1..100000 -- still 100000 groups, just unsorted,
+# so the hash table grows and spills.
+#
+# Each query aggregates over the grouped result, so the expected output is one
+# row. sum(1..100000) = 5000050000, and every v lands in one group, so the
+# per-group sums always add back to that total.
+
+# Single partition keeps the aggregation in one operator (no repartition).
+statement ok
+SET datafusion.execution.target_partitions = 1
+
+statement ok
+SET datafusion.runtime.memory_limit = '1M'
+
+# --- Case A: single-column high-cardinality GROUP BY ---
+query II
+SELECT count(*), sum(total)
+FROM (
+ SELECT (v * 7) % 100000 AS k, sum(v) AS total
+ FROM generate_series(1, 100000) AS t(v)
+ GROUP BY (v * 7) % 100000
+)
+----
+100000 5000050000
+
+# Prove the inner aggregate actually spills (else these tests would silently stop covering the spill path).
+# Only `spill_count` is pinned; the other metrics vary per run.
Review Comment:
Let's assert the `spill_count` similarly in other queries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test (slt): add memory-limited aggregation sqllogictests [datafusion]
Posted by "naman-modi (via GitHub)" <gi...@apache.org>.
naman-modi commented on PR #23838:
URL: https://github.com/apache/datafusion/pull/23838#issuecomment-5068725868
Thanks for the review @2010YOUY01! I have addressed the review comments, and also fixed the CI (passes in local testing atleast). Can you please help me trigger the CI workflow again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test (slt): add memory-limited aggregation sqllogictests [datafusion]
Posted by "naman-modi (via GitHub)" <gi...@apache.org>.
naman-modi commented on code in PR #23838:
URL: https://github.com/apache/datafusion/pull/23838#discussion_r3644634649
##########
datafusion/sqllogictest/test_files/aggregate_memory_limit.slt:
##########
@@ -0,0 +1,137 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Memory-limited (spilling) grouped hash aggregation.
+#
+# High-cardinality GROUP BY under a tight memory limit: the aggregate spills to
+# disk, re-groups the spilled state, and must still return the right answer.
+#
+# The group key is scrambled with `(v * 7) % 100000` because generate_series is
+# sorted, which would take the streaming path that never spills. gcd(7, 100000)
+# = 1, so it's a bijection over 1..100000 -- still 100000 groups, just unsorted,
+# so the hash table grows and spills.
+#
+# Each query aggregates over the grouped result, so the expected output is one
+# row. sum(1..100000) = 5000050000, and every v lands in one group, so the
+# per-group sums always add back to that total.
+
+# Single partition keeps the aggregation in one operator (no repartition).
+statement ok
+SET datafusion.execution.target_partitions = 1
+
+statement ok
+SET datafusion.runtime.memory_limit = '1M'
+
+# --- Case A: single-column high-cardinality GROUP BY ---
+query II
+SELECT count(*), sum(total)
+FROM (
+ SELECT (v * 7) % 100000 AS k, sum(v) AS total
+ FROM generate_series(1, 100000) AS t(v)
+ GROUP BY (v * 7) % 100000
+)
+----
+100000 5000050000
+
+# Prove the inner aggregate actually spills (else these tests would silently stop covering the spill path).
+# Only `spill_count` is pinned; the other metrics vary per run.
Review Comment:
Done, added a spill_count assertion to each case now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test (slt): add memory-limited aggregation sqllogictests [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan merged PR #23838:
URL: https://github.com/apache/datafusion/pull/23838
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test (slt): add memory-limited aggregation sqllogictests [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23838:
URL: https://github.com/apache/datafusion/pull/23838#issuecomment-5067613693
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23838?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.74%. Comparing base ([`7fb2e2c`](https://app.codecov.io/gh/apache/datafusion/commit/7fb2e2c7a6acbad5443c2631d7232b340160fd86?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`45e1a39`](https://app.codecov.io/gh/apache/datafusion/commit/45e1a398b5a15d7ddbbd9519aa66307824232c01?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 18 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #23838 +/- ##
==========================================
- Coverage 80.75% 80.74% -0.01%
==========================================
Files 1089 1089
Lines 368824 368824
Branches 368824 368824
==========================================
- Hits 297829 297821 -8
- Misses 53246 53249 +3
- Partials 17749 17754 +5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23838?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org