Repository navigation
Split src/parser/mod.rs and src/ast/mod.rs into smaller modules #2591
Description
Activity
I strongly support this approach. The incremental pure-move strategy is definitely the right path forward to avoid reviewer fatigue and prevent merge conflicts with ongoing dialect PRs.
A couple of quick points to ensure smooth execution:
- Zero Breaking Changes / API Compatibility: We should ensure
src/parser/mod.rsandsrc/ast/mod.rsre-export all moved items (pub use module::*;) so downstream consumers (like DataFusion, Polars, ParadeDB, etc.) experience zero breaking changes. - Pure Move Verification Script: Before starting the refactor PRs, we can write a lightweight CI check/script (e.g., checking symbol re-exports and git line churn) to guarantee that moves are strictly structural with zero logic modifications.
I’d be glad to take this on! I can start by drafting the pure-move verification script or handling the first incremental module split (e.g., extracting a specific logical sub-parser from
src/parser/mod.rs).Please feel free to assign this to me, or let me know where you'd prefer to begin!
- Zero Breaking Changes / API Compatibility: We should ensure
take
@mohitgurav20 Just like #2587, this is an issue to get agreement from the other maintainers. If agreed, it will require stacked PRs, which need write access.
The comment above restates this issue back to it, like most of the analyses you posted with
takeon nine apache/datafusion issues in three days. Using LLMs as coding support is fine, they are great tools. What is not okay is using them blindly or posting their output directly to people and maintainers. The DataFusion AI policy discourages that behaviour. Please read https://gruhn.me/blog/2026-08-03/ and do not post generated comments here.If you want to help, we are trying to chip away at existing bugs using the fuzzing harnesses I added. Feel free to start running the fuzzer locally on your machine for any of them. Nowadays generating code is extremely cheap and we could all do it. The hard part is making sure it is correct, and we are at capacity with our ability to do reviews.
@alamb as I mentioned in #2588, I believe we should update the AI policy in our AGENTS.md to discourage this type of behaviour, as the rust-lang core repositories have started doing in their PR templates following https://forge.rust-lang.org/policies/llm-usage.html
Archived copies of the nine claims and the three PRs
Wayback Machine snapshots taken on 2026-09-27.
- Propagate window ranking statistics into rank filters datafusion#25622 https://web.archive.org/web/20260927064142/https://github.com/apache/datafusion/issues/25622
- Decorrelate subqueries whose grouping sets leave out the correlated column datafusion#25708 https://web.archive.org/web/20260927064357/https://github.com/apache/datafusion/issues/25708
- Chained CollectLeft hash joins multiply StringView buffer references (TPC-DS Q64: 8.5 GB, ~10x slower with pushdown_filters) datafusion#25712 https://web.archive.org/web/20260927064425/https://github.com/apache/datafusion/issues/25712
- Substrait consumer rejects multi-needle InPredicate for row-valued IN subqueries datafusion#25720 https://web.archive.org/web/20260927064453/https://github.com/apache/datafusion/issues/25720
- Use Exact Hash-Table Allocation Accounting in
GroupValuesPrimitivedatafusion#25734 https://web.archive.org/web/20260927064522/https://github.com/apache/datafusion/issues/25734 - Use Exact Hash-Table Allocation Accounting in
GroupValuesColumndatafusion#25736 https://web.archive.org/web/20260927064736/https://github.com/apache/datafusion/issues/25736 - fix: deduplicate StringView/BinaryView buffer refs in CollectLeft Has... datafusion#25716 https://web.archive.org/web/20260927064948/https://github.com/apache/datafusion/pull/25716
- fix: use NDV for unresolved scalar subquery selectivity instead of 20% fallback datafusion#25719 https://web.archive.org/web/20260927065021/https://github.com/apache/datafusion/pull/25719
- fix(physical-plan): replace approximate
map_sizeaccounting with exactHashTable::allocation_size()inGroupValuesRowsdatafusion#25750 https://web.archive.org/web/20260927065049/https://github.com/apache/datafusion/pull/25750
The Wayback Machine did not capture these three, so they link the comments directly.
- Estimate equality to a scalar subquery without a blanket 20% fallback datafusion#25623 Estimate equality to a scalar subquery without a blanket 20% fallback datafusion#25623 (comment)
- Use Exact Hash-Table Allocation Accounting in
GroupValuesRowsdatafusion#25735 Use Exact Hash-Table Allocation Accounting inGroupValuesRowsdatafusion#25735 (comment) - Allow DataSouceExec to have child nodes that supply files at runtime datafusion#25775 Allow DataSouceExec to have child nodes that supply files at runtime datafusion#25775 (comment)
Reacted by Mohit GuravThanks for the feedback @LucaCappelletti94. Apologies for the noise—I appreciate you pointing out the policy and the review bandwidth constraints.
I’ll focus on running the fuzzers locally and working directly on verified code fixes and test cases going forward. Thanks for your time and maintainership!
I suggest we try to do this before the next release, currently roughly planned for end of October (#2454), as a series of pure move PRs. I believe that, given the size of the task, it may be desirable to:
Make sense to me
Do this operation in single, reasonably sized PRs
Prepare BEFORE a script for the CI that checks that the move is actually a move, and it is completeI can just use AI tools for this verification, I don't think a special script is necessary (at least for me) to review
- marked Suggest splitting
parser.rsinto smaller mod files #944 as a duplicate of this issueon Sep 28, 2026
Several source files have grown past the point where navigating them is practical, with the parser mod at over 20k and the ast mod at over 10k.
This has come up before. #944 (2023, still open) proposed one parser file per statement, and #1581 (2024) split the whole parser in one draft PR. Both got agreement on the goal, conditioned on a series of small pure-move PRs. The earlier one-shot attempts #344 and #351 (2021) stalled on reviewer bandwidth, with concerns about conflicts with open PRs and about coupled code being easier to read in one file.
I suggest we try to do this before the next release, currently roughly planned for end of October (#2454), as a series of pure move PRs. I believe that, given the size of the task, it may be desirable to:
If agreed, this supersedes #944.