chore: deprecate ValuesExec in favour of MemoryExec#14032
Conversation
github-actions
bot
added
physical-expr
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @jonathanc-n -- this one is looking nice 👌
| .collect::<Result<Vec<Arc<dyn PhysicalExpr>>>>() | ||
| }) | ||
| .collect::<Result<Vec<_>>>()?; | ||
| #[allow(deprecated)] // TODO: Remove in favour of MemoryExec |
There was a problem hiding this comment.
In this PR I think it would be good to port this code (in the physical planner) to use MemoryExec -- that way queries will run through MemoryExec and we will confidence that ValuesExec can really be removed
Co-authored-by: Andrew Lamb <[email protected]>
| ) | ||
| } | ||
|
|
||
| fn compute_properties_as_value(schema: SchemaRef) -> PlanProperties { |
There was a problem hiding this comment.
I think compute_properties could be reuse here. vec![batches] is len=1, pass ordering with &[].
There was a problem hiding this comment.
is this something we should do prior to merging the PR?
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @jonathanc-n and @jayzhan211
The only thing I am not sure if we should do prior to merge is @jayzhan211's comment
https://github.com/apache/datafusion/pull/14032/files#r1907047878
| .collect::<Result<Vec<_>>>()?; | ||
| let value_exec = ValuesExec::try_new(SchemaRef::new(exec_schema), exprs)?; | ||
| let value_exec = | ||
| MemoryExec::try_new_as_values(SchemaRef::new(exec_schema), exprs)?; |
| ) | ||
| } | ||
|
|
||
| fn compute_properties_as_value(schema: SchemaRef) -> PlanProperties { |
There was a problem hiding this comment.
is this something we should do prior to merging the PR?
|
Thanks again @jonathanc-n and @jayzhan211 -- I merged up to resolve a conflict and will plan to merge when the tests are clean |
|
🚀 |
|
BTW we got some feedback that the deprecation message could be improved on this PR. Here is a proposal: |
3 participants
Which issue does this PR close?
Closes #13968 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?