Skip to content

Stop DataFusionToolset from materializing full results for a capped query - #73384

Merged
kaxil merged 2 commits into
apache:mainfrom
ColtenOuO:datafusion-toolset-limit-rows
Sep 24, 2026
Merged

kaxil merged 2 commits into
apache:mainfrom
ColtenOuO:datafusion-toolset-limit-rows

Conversation

@ColtenOuO

@ColtenOuO ColtenOuO commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

DataFusionToolset's query tool returns at most max_rows rows (default 50), but it ran the query unbounded and called to_pydict() on the whole result before slicing, so a broad query like SELECT * FROM big_table could exhaust worker memory. The query is now capped at max_rows + 1 rows with a DataFusion LIMIT before anything is materialized; the extra row only signals truncation. What the agent sees is unchanged apart from total_rows (see below).

Changes

  • Run the query as session_context.sql(sql).limit(max_rows + 1).to_pydict() and stop reporting total_rows. Errors are wrapped in QueryExecutionException with the same message DataFusionEngine.execute_query produces, so the retry handling is unchanged.
  • DataFusionEngine.execute_query gained a max_rows argument only in common-sql 1.34.0, while this provider supports >=1.33.0. Applying the limit on the DataFrame keeps the change working on 1.33.0 without bumping the dependency.
  • Update the tests to mock the DataFrame chain and assert the limit(max_rows + 1) call.

Impact

Peak RSS of one query call on a local 3M-row parquet file (65 MiB), max_rows=50:

Query Before After Time before → after
SELECT * 1121 MiB 219 MiB 4.20s → 0.05s
WHERE amount > 500 860 MiB 218 MiB 1.16s → 0.07s
ORDER BY amount DESC 1358 MiB 279 MiB 3.30s → 0.30s
GROUP BY (20 rows) 219 MiB 219 MiB unchanged

Before, memory grew linearly with the result size; after, it no longer depends on the result's row count.

Trade-off

total_rows is no longer returned, since only max_rows + 1 rows are read, and knowing the total would require a full scan or a second COUNT(*) query. An untruncated result loses nothing because row_count is already the total. For a truncated result, an agent that needs the count can run COUNT(*), in line with the tool description and hint, which already tell it to aggregate in SQL rather than page through a truncated result. SQLToolset already omits total_rows whenever the driver can't report a trustworthy total.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code (Opus 5) following the guidelines

@ColtenOuO
ColtenOuO marked this pull request as draft September 19, 2026 17:28
@ColtenOuO
ColtenOuO force-pushed the datafusion-toolset-limit-rows branch 3 times, most recently from b9ce82c to f9fef23 Compare September 19, 2026 21:19
@ColtenOuO
ColtenOuO marked this pull request as ready for review September 20, 2026 08:01
@ColtenOuO
ColtenOuO requested a review from kaxil September 20, 2026 15:32
The bounded-results documentation still described the old behaviour, where
the engine materialized the whole result before the toolset bounded it, so
it now contradicted the code and the missing total_rows. The row limit also
makes EXPLAIN fail, which is only reachable with allow_writes=True and is
easier to meet in the docstring than in an error from the agent.
@ColtenOuO
ColtenOuO force-pushed the datafusion-toolset-limit-rows branch from 5200f44 to 53bdec9 Compare September 24, 2026 03:27
@ColtenOuO

Copy link
Copy Markdown
Contributor Author

Resolve the conflict :D

@Lee-W Lee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit

@kaxil
kaxil merged commit 7722b1b into apache:main Sep 24, 2026
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants