Skip to content

Conversation

@mkleen
Copy link
Contributor

@mkleen mkleen commented Oct 30, 2025

Which issue does this PR close?

Rationale for this change

This moves the projection logic from generate_series out of the generator into LazyMemoryStream as discussed in #18298 (comment) This makes the projection logic generic for all generators.

What changes are included in this PR?

The projection logic is moved from generate_series into the LazyMemoryStream and relevant tests, where LazyMemoryStream is used, are adapted accordingly.

Are these changes tested?

This is only a small refactoring; the changes are covered by the tests from #18298

Are there any user-facing changes?

There is a new parameter added to LazyMemoryExec::try_new method

@github-actions github-actions bot added core Core DataFusion crate proto Related to proto crate functions Changes to functions implementation physical-plan Changes to the physical-plan crate labels Oct 30, 2025
@mkleen mkleen force-pushed the mkleen/move-projection-to-stream branch from 4cee5a0 to a1f2b03 Compare October 30, 2025 06:06
@mkleen mkleen force-pushed the mkleen/move-projection-to-stream branch from a1f2b03 to b3b3afe Compare October 30, 2025 06:17
@mkleen mkleen marked this pull request as ready for review October 30, 2025 08:48
@alamb alamb added the api change Changes the API exposed to users of the crate label Oct 31, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @mkleen -- this looks like a nice cleanup to me

I have one comment on the API -- let me know what you think

/// Create a new lazy memory execution plan
pub fn try_new(
schema: SchemaRef,
projection: Option<Vec<usize>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is technically a breaking API change -- it is minor but still breaking

What would you think about adding a new method like

impl LazyMemoryExec {
  pub fn with_projection(projection: Option<Vec<usize>>) -> Self { ... }
}

instead and leave the signature of try_new the same

That would avoid the API breakage (and likely reduce the size of this PR a bit as many callsites would not have to be changed)

Copy link
Contributor Author

@mkleen mkleen Nov 1, 2025

Choose a reason for hiding this comment

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

I agree, thanks for the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate core Core DataFusion crate functions Changes to functions implementation physical-plan Changes to the physical-plan crate proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants