You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Tpt (via GitHub)" <gi...@apache.org> on 2025/11/29 17:43:57 UTC
[PR] CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Tpt opened a new pull request, #18993:
URL: https://github.com/apache/datafusion/pull/18993
It was previously ignored
## Which issue does this PR close?
- Closes #18992.
## Rationale for this change
All `TableProvider` implementations must support the `projection` argument of the `scan` method. This was not the case in `CteWorkTable`.
## What changes are included in this PR?
Minimal implementation of the projection support. The projection applied before the plan node return results. It might be nice to push it further inside of the recursion implementation to reduce memory consumption but I prefered to keep the fix minimal.
## Are these changes tested?
I have not figured out yet a nice SQL query to trigger an error without this change. Some existing queries in `cte.slt` have set projection (i.e. not `None`) so the code is very likely working.
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2615461877
##########
datafusion/catalog/src/cte_worktable.rs:
##########
@@ -86,15 +87,23 @@ impl TableProvider for CteWorkTable {
async fn scan(
&self,
_state: &dyn Session,
- _projection: Option<&Vec<usize>>,
- _filters: &[Expr],
- _limit: Option<usize>,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
Review Comment:
Sadly the `scan` method does not have a default implementation in the trait so I have to implement it. I preferred to make it call `scan_with_args` but glad to do otherwise if you peefer.
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2615519750
##########
datafusion/physical-plan/src/work_table.rs:
##########
@@ -196,11 +206,22 @@ impl ExecutionPlan for WorkTableExec {
0,
"WorkTableExec got an invalid partition {partition} (expected 0)"
);
- let batch = self.work_table.take()?;
+ let ReservedBatches {
+ mut batches,
+ reservation,
+ } = self.work_table.take()?;
+ if let Some(projection) = &self.projection {
+ // We apply the projection
+ // TODO: it would be better to apply it as soon as possible and not only here
+ // TODO: an aggressive projection makes the memory reservation smaller, even if we do not edit it
Review Comment:
Could you expand on this a bit? Where would "as soon as possible" be? I'm not familiar with what WorkTableExec is or how it is used 🥲
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2615452567
##########
datafusion/catalog/src/cte_worktable.rs:
##########
@@ -86,15 +87,23 @@ impl TableProvider for CteWorkTable {
async fn scan(
&self,
_state: &dyn Session,
- _projection: Option<&Vec<usize>>,
- _filters: &[Expr],
- _limit: Option<usize>,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
Review Comment:
Not quite what I meant: you'd implement `scan_with_args` and delete the `scan` implementation
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2612158841
##########
datafusion/catalog/src/cte_worktable.rs:
##########
@@ -86,15 +87,23 @@ impl TableProvider for CteWorkTable {
async fn scan(
&self,
_state: &dyn Session,
- _projection: Option<&Vec<usize>>,
- _filters: &[Expr],
- _limit: Option<usize>,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
Review Comment:
May I recommend we update to use `scan_with_args`?
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#issuecomment-3648027954
Is there a SQL reproducer for this? Could we add slt tests?
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2615461877
##########
datafusion/catalog/src/cte_worktable.rs:
##########
@@ -86,15 +87,23 @@ impl TableProvider for CteWorkTable {
async fn scan(
&self,
_state: &dyn Session,
- _projection: Option<&Vec<usize>>,
- _filters: &[Expr],
- _limit: Option<usize>,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
Review Comment:
Sadly the `scan` method does not have a default implementation in the trait so I have to implement it. I preferred to make it call `scan_with_args` but glad to do otherwise if you prefer.
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2615470178
##########
datafusion/catalog/src/cte_worktable.rs:
##########
@@ -86,15 +87,23 @@ impl TableProvider for CteWorkTable {
async fn scan(
&self,
_state: &dyn Session,
- _projection: Option<&Vec<usize>>,
- _filters: &[Expr],
- _limit: Option<usize>,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
Review Comment:
Ah yes right I see. I don't think it will ever be called if you implement `scan_with_args`. But it's okay to leave the implementation here as you have.
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#issuecomment-3648037491
> Is there a SQL reproducer for this? Could we add slt tests?
I spent more than an hour trying to reproduce it in SQL but failed, sadly. It's why I wrote an explicit test for projection. I encountered this issue while implementing SPARQL property paths on top of DataFusion.
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#issuecomment-3692611975
I have rebased the MR after the merge of #18956
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2573941082
##########
datafusion/physical-plan/src/work_table.rs:
##########
@@ -103,25 +103,35 @@ pub struct WorkTableExec {
name: String,
/// The schema of the stream
schema: SchemaRef,
+ /// Projection to apply to build the output stream from the recursion state
+ projection: Option<Vec<usize>>,
/// The work table
work_table: Arc<WorkTable>,
/// Execution metrics
metrics: ExecutionPlanMetricsSet,
/// Cache holding plan properties like equivalences, output partitioning etc.
Review Comment:
Indeed, I have moved back the field name to `cache` to stay consistent with a lot of other operator implementations: 69464d2b73919d79fdf8176dbfa7645ca66e5e2f
##########
datafusion/physical-plan/src/work_table.rs:
##########
Review Comment:
Done! 304d40c2484e8873abbfd33be80a957be1e1257b
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #18993:
URL: https://github.com/apache/datafusion/pull/18993
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2613290604
##########
datafusion/catalog/src/cte_worktable.rs:
##########
@@ -86,15 +87,23 @@ impl TableProvider for CteWorkTable {
async fn scan(
&self,
_state: &dyn Session,
- _projection: Option<&Vec<usize>>,
- _filters: &[Expr],
- _limit: Option<usize>,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
Review Comment:
Done!
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#issuecomment-3695342077
Thanks @Tpt, @martin-g & @adriangb
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2615534158
##########
datafusion/physical-plan/src/work_table.rs:
##########
@@ -196,11 +206,22 @@ impl ExecutionPlan for WorkTableExec {
0,
"WorkTableExec got an invalid partition {partition} (expected 0)"
);
- let batch = self.work_table.take()?;
+ let ReservedBatches {
+ mut batches,
+ reservation,
+ } = self.work_table.take()?;
+ if let Some(projection) = &self.projection {
+ // We apply the projection
+ // TODO: it would be better to apply it as soon as possible and not only here
+ // TODO: an aggressive projection makes the memory reservation smaller, even if we do not edit it
Review Comment:
`WorkTableExec` is used for recursive CTEs. They are evaluated by executing the same query plan multiple time. The output of the previous iteration is injected as the value of the "work table" in the next iteration.
Here "as soon as possible" means that when we execute the previous iteration we don't need to save all the columns of the output of this iteration to feed them into the "work table" of the next iteration but only the columns that are actually needed.
--
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] fix: CteWorkTable: properly apply TableProvider::scan projection argument [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #18993:
URL: https://github.com/apache/datafusion/pull/18993#discussion_r2573766569
##########
datafusion/catalog/src/cte_worktable.rs:
##########
@@ -86,15 +87,20 @@ impl TableProvider for CteWorkTable {
async fn scan(
&self,
_state: &dyn Session,
- _projection: Option<&Vec<usize>>,
- _filters: &[Expr],
- _limit: Option<usize>,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
- // TODO: pushdown filters and limits
+ assert_or_internal_err!(
+ filters.is_empty(),
+ "CteWorkTable does not support pushing filters"
+ );
+ assert_or_internal_err!(limit.is_none(), "CteWorkTable pushing limit");
Review Comment:
The error message is a bit unclear to me.
Maybe:
```suggestion
assert_or_internal_err!(limit.is_none(), "CteWorkTable does not support limit pushdown");
```
?!
##########
datafusion/physical-plan/src/work_table.rs:
##########
Review Comment:
It would be good to add a test case using projection.
##########
datafusion/physical-plan/src/work_table.rs:
##########
@@ -103,25 +103,35 @@ pub struct WorkTableExec {
name: String,
/// The schema of the stream
schema: SchemaRef,
+ /// Projection to apply to build the output stream from the recursion state
+ projection: Option<Vec<usize>>,
/// The work table
work_table: Arc<WorkTable>,
/// Execution metrics
metrics: ExecutionPlanMetricsSet,
/// Cache holding plan properties like equivalences, output partitioning etc.
Review Comment:
The docstring is outdated now. The field is not named `cache` anymore.
--
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