Skip to content

Conversation

@Tpt
Copy link
Contributor

@Tpt Tpt commented Nov 29, 2025

It was previously ignored

Which issue does this PR close?

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 preferred 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. There is also a test on the projection itself in WorkTableExec

@github-actions github-actions bot added catalog Related to the catalog crate physical-plan Changes to the physical-plan crate labels Nov 29, 2025
@Tpt Tpt marked this pull request as ready for review November 29, 2025 20:39
@Tpt Tpt changed the title CteWorkTable: properly apply TableProvider::scan projection argument fix: CteWorkTable: properly apply TableProvider::scan projection argument Nov 29, 2025
drop(memory_stream);
assert_eq!(pool.reserved(), 0);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add a test case using projection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! 304d40c

work_table: Arc<WorkTable>,
/// Execution metrics
metrics: ExecutionPlanMetricsSet,
/// Cache holding plan properties like equivalences, output partitioning etc.
Copy link
Member

Choose a reason for hiding this comment

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

The docstring is outdated now. The field is not named cache anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I have moved back the field name to cache to stay consistent with a lot of other operator implementations: 69464d2

@Tpt Tpt force-pushed the tpt/cteworktable-projection branch from da29338 to 304d40c Compare November 30, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

catalog Related to the catalog crate physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CteWorkTable does not properly apply projection

2 participants