Fix: Add projection to generate_series #18298
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
sqllogictest
mkleen
changed the title
mkleen
force-pushed
the
fix-projection-generate-series
branch
from
e605743 to
589bd3d
Compare
mkleen
force-pushed
the
fix-projection-generate-series
branch
from
589bd3d to
3cbb00e
Compare
mkleen
commented
Oct 27, 2025
| let array = self.current.create_array(buf)?; | ||
| let batch = RecordBatch::try_new(Arc::clone(&self.schema), vec![array])?; | ||
| Ok(Some(batch)) | ||
| let projected = match self.projection.as_ref() { |
There was a problem hiding this comment.
I noticed that the MemoryStream projects in https://github.com/apache/datafusion/blob/main/datafusion/physical-plan/src/memory.rs#L103 while the LazyMemoryStream used in generate_series doesn't. An alternative approach could be to project generically in the LazyMemoryStream instead inside of the generator.
There was a problem hiding this comment.
That sounds like a great idea. Perhaps you could do so as a follow on PR
alamb
approved these changes
Oct 27, 2025
xudong963
approved these changes
Oct 28, 2025
|
Thanks @mkleen and @xudong963 |
tobixdev
pushed a commit
to tobixdev/datafusion
that referenced
this pull request
Nov 2, 2025
## Which issue does this PR close? - Closes apache#17830 ## Rationale for this change The queries from the original ticket fail, because an unprojected `generate_series` function would produce in a join the wrong number of columns which leads to a runtime error. ## What changes are included in this PR? This adds a missing projection to `generate_series` to ensure values are only emitted when projected. ## Are these changes tested? I added a sql-logic test. I also compared the results against Postgres and DuckDB: Postgres: ```sql mkleen=# SELECT v1 FROM (select generate_series as v1 from generate_series(1, 3)) g1, (select generate_series as v2 from generate_series(1, 3)) g2; v1 ---- 1 1 1 2 2 2 3 3 3 (9 rows) ``` DuckDB: ```sql D SELECT v1 FROM (select generate_series as v1 from generate_series(1, 3)) g1, (select generate_series as v2 from generate_series(1, 3)) g2; ┌───────┐ │ v1 │ │ int64 │ ├───────┤ │ 1 │ │ 2 │ │ 3 │ │ 1 │ │ 2 │ │ 3 │ │ 1 │ │ 2 │ │ 3 │ └───────┘ ``` ## Are there any user-facing changes? No
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 4, 2025
## Which issue does this PR close? - None, This is a follow-up for #18298 ## 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
jizezhang
pushed a commit
to jizezhang/datafusion
that referenced
this pull request
Nov 5, 2025
…8373) ## Which issue does this PR close? - None, This is a follow-up for apache#18298 ## Rationale for this change This moves the projection logic from `generate_series` out of the generator into `LazyMemoryStream` as discussed in apache#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 apache#18298 ## Are there any user-facing changes? There is a new parameter added to LazyMemoryExec::try_new method
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
generate_series: Invalid argument error: number of columns must match number of fields in schema #17830Rationale for this change
The queries from the original ticket fail, because an unprojected
generate_seriesfunction would produce in a join the wrong number of columns which leads to a runtime error.What changes are included in this PR?
This adds a missing projection to
generate_seriesto ensure values are only emitted when projected.Are these changes tested?
I added a sql-logic test. I also compared the results against Postgres and DuckDB:
Postgres:
DuckDB:
Are there any user-facing changes?
No