Add `TableProvider::scan_with_args` by adriangb · Pull Request #17336 · apache/datafusion

adriangb · 2025-08-27T19:36:09Z

This refactor will allow us to evolve the scan() API while minimizing API churn, e.g. to:

Incorporate sorting information from Push down preferred sorts into TableScan logical plan node #17337
Allow scan() to more easily evaluate some filters (Add TableProvider::scan_with_args to support pushdown sorting #17273 (comment))

adriangb · 2025-09-05T16:23:19Z

+ let options = ScanArgs::default()
+ .with_projection(projection.cloned())
+ .with_filters(Some(filters.to_vec()))
+ .with_limit(limit);
+ Ok(self.scan_with_args(state, options).await?.plan())
+ }


I think the biggest question here is how to handle backwards compatibility. I implemented both methods in terms of the other which is nice because:
Existing users are not forced to implement scan_with_args
We can start using scan_with_args in the physical planner.
But if you implement neither I think you'll get a recursion error.
I also chose not to deprecate scan() to (1) avoid a bunch of code churn mostly for us and (2) until we are confident that scan_with_args is a workable evolution

I think the recursion error is unfortunate -- maybe we could avoid problems with good enough documentation but it seems like a rough edge on one of the core experiences (TableProvider)
Here is an alternate proposal:
leave scan required (and leave scan_with_args in terms of scan)
Once we are happy with scan_with_args we can then properly deprecate (maybe even remove scan) in favor of scan_with_args as a follow on PR (in some future PR)
This approach has the downside that it is more complicated to implement scan_with_args as you will be forced to also provide a not implemented error or something.
The upside is that there is no change needed to existing code and those who aren't yet interested in scan_with_args won't need a change
I also chose not to deprecate scan() to (1) avoid a bunch of code churn mostly for us and (2) until we are confident that scan_with_args is a workable evolution
This seems like a good idea to me

adriangb · 2025-09-10T20:34:15Z

@alamb could you give this a look? I don't think this is blocked by anything and we already discussed it a bit offline.

alamb · 2025-09-11T14:16:32Z

@alamb could you give this a look? I don't think this is blocked by anything and we already discussed it a bit offline.

I will try and review it either later today or tomorrow

alamb

Thanks @adriangb and @xudong963 -- in general I think this PR is a nice improvement and step in the right direction.

I had some small API suggestions (mostly to avoid cloning)

my only blocking concern the infinite recursion problem -- I left a suggestion for how to address it. Let me know what you think

alamb · 2025-09-12T12:45:08Z

+ let options = ScanArgs::default()
+ .with_projection(projection.cloned())
+ .with_filters(Some(filters.to_vec()))
+ .with_limit(limit);
+ Ok(self.scan_with_args(state, options).await?.plan())
+ }


I think the recursion error is unfortunate -- maybe we could avoid problems with good enough documentation but it seems like a rough edge on one of the core experiences (TableProvider)
Here is an alternate proposal:
leave scan required (and leave scan_with_args in terms of scan)
Once we are happy with scan_with_args we can then properly deprecate (maybe even remove scan) in favor of scan_with_args as a follow on PR (in some future PR)
This approach has the downside that it is more complicated to implement scan_with_args as you will be forced to also provide a not implemented error or something.
The upside is that there is no change needed to existing code and those who aren't yet interested in scan_with_args won't need a change
I also chose not to deprecate scan() to (1) avoid a bunch of code churn mostly for us and (2) until we are confident that scan_with_args is a workable evolution
This seems like a good idea to me

alamb · 2025-09-12T12:46:07Z

+ ///
+ /// Returns a cloned copy of the projection column indices, or `None` if
+ /// no projection was specified (meaning all columns should be included).
+ pub fn projection(&self) -> Option<Vec<usize>> {


I think this should return a reference (Option<&Vec<usize>>) to avoid forcing clones even if the caller doesn't need a copy

Suggested change
pubfn projection(&self) -> Option<Vec<usize>>{
pubfn projection(&self) -> Option<&Vec<usize>>{

alamb · 2025-09-12T12:46:47Z

+ /// Get the filter expressions for the scan.
+ ///
+ /// Returns a reference to the filter expressions, or `None` if no filters were specified.
+ pub fn filters(&self) -> Option<&[Expr]> {


(minor) I recommend returing the raw Vec reference so the caller can use Vec specific functions
Suggested change
pubfn filters(&self) -> Option<&[Expr]>{
pubfn filters(&self) -> Option<&Vec<Expr>>{

alamb · 2025-09-12T12:47:57Z

+/// `ScanResult` encapsulates the [`ExecutionPlan`] produced by a table scan,
+/// providing a typed return value instead of returning the plan directly.
+/// This allows for future extensibility of scan results without breaking
+/// the API.


How much value does this comment add? While accurate I don't think it provides much value and just makes the code harder to read (sounds like an LLM wrote it lol)

alamb · 2025-09-12T12:48:51Z

+ ///
+ /// Returns a cloned reference to the [`ExecutionPlan`] that will perform
+ /// the actual table scanning and data retrieval.
+ pub fn plan(&self) -> Arc<dyn ExecutionPlan> {


I also recommend here providing this as a reference as well as providing a way to get the plan without a clone
Suggested change
pubfn plan(&self) -> Arc<dynExecutionPlan>{
pubfn plan(&self) -> &Arc<dynExecutionPlan>{
And then also add a function like ScanResult::into_inner to return the inner plan

Another nice usability thing would be to provide a From impl so we can make thse like ScanResult::from(plan)

alamb · 2025-09-12T12:51:01Z

- .scan(session_state, projection.as_ref(), &filters, *fetch)
- .await?
+ let opts = ScanArgs::default()
+ .with_projection(projection.clone())


is there a reason we need to cone the projection here? I think it would be better and avoid the clone and pass in the arguments via reference (so something like ScanArgs<'a>)

adriangb · 2025-09-12T20:11:59Z

@alamb I believe I've addressed your feedback, thank you for the review

alamb

Looks good to me -- thank you @adriangb

I left some small suggestions, but nothing we can't fix after merge (or never)

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actionsBot added core Core DataFusion crate catalog Related to the catalog crate labels Aug 27, 2025

adriangb commented Aug 27, 2025
View reviewed changes

Comment threaddatafusion/catalog/src/table.rs Outdated

adriangb mentioned this pull request Aug 27, 2025
Add TableProvider::scan_with_args to support pushdown sorting #17273
Closed

adriangb force-pushed the refactor-scan-to-scan-with-args branch from 2818549 to f3273b7Compare August 27, 2025 19:48

adriangb mentioned this pull request Aug 28, 2025
[EPIC] Sort pushdown / partially sorted scans #17348
Closed

adriangb force-pushed the refactor-scan-to-scan-with-args branch 2 times, most recently from 96ea7e8 to a523803Compare September 4, 2025 17:45

xudong963 reviewed Sep 5, 2025
View reviewed changes

Comment threaddatafusion/catalog/src/table.rs Outdated

adriangb requested review from alamb and xudong963 September 5, 2025 16:21

adriangb commented Sep 5, 2025
View reviewed changes

alamb reviewed Sep 12, 2025
View reviewed changes

alamb changed the title ~~Refactor TableProvider::scan into TableProvider::scan_with_args~~Add TableProvider::scan_with_argsSep 12, 2025

adriangb added 3 commits September 12, 2025 10:35

Refactor TableProvider::scan into TableProvider::scan_with_args
53bc15b

lint
5278349

fix
1d5bad1

adriangb force-pushed the refactor-scan-to-scan-with-args branch from 58845c0 to 1d5bad1Compare September 12, 2025 16:16

alamb approved these changes Sep 12, 2025
View reviewed changes

Comment threaddatafusion/catalog/src/table.rs Outdated
Comment threaddatafusion/catalog/src/table.rs Outdated
Comment threaddatafusion/core/src/datasource/listing/table.rs Outdated

adriangband others added 4 commits September 12, 2025 15:20

Update datafusion/catalog/src/table.rs
b7d00a3
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/core/src/datasource/listing/table.rs
550cf26
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/catalog/src/table.rs
46967c7
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

fmt
184f15a

adriangb merged commit 7b45934 into apache:mainSep 15, 2025
28 checks passed

adriangb deleted the refactor-scan-to-scan-with-args branch September 15, 2025 19:00

adriangb added a commit to pydantic/datafusion that referenced this pull request Oct 27, 2025
AddTableProvider::scan_with_args(apache#17336)
04926ae

	pubfn projection(&self) -> Option<Vec<usize>>{
	pubfn projection(&self) -> Option<&Vec<usize>>{

	pubfn filters(&self) -> Option<&[Expr]>{
	pubfn filters(&self) -> Option<&Vec<Expr>>{

	pubfn plan(&self) -> Arc<dynExecutionPlan>{
	pubfn plan(&self) -> &Arc<dynExecutionPlan>{

Uh oh!

Conversation

adriangb commented Aug 27, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Sep 10, 2025

Uh oh!

alamb commented Sep 11, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Sep 12, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adriangb commented Aug 27, 2025•
edited
Loading