chore: deprecate ValuesExec in favour of MemoryExec
#14032
Conversation
github-actions
bot
added
physical-expr
| @@ -466,6 +467,7 @@ impl DefaultPhysicalPlanner { | |||
| .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
There was a problem hiding this comment.
@alamb Should be fine now
Co-authored-by: Andrew Lamb <[email protected]>
| @@ -293,6 +383,15 @@ impl MemoryExec { | |||
| Boundedness::Bounded, | |||
| ) | |||
| } | |||
|
|
|||
| 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?
There was a problem hiding this comment.
@alamb Yes, just added it
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
| @@ -466,7 +465,8 @@ impl DefaultPhysicalPlanner { | |||
| .collect::<Result<Vec<Arc<dyn PhysicalExpr>>>>() | |||
| }) | |||
| .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)?; | |||
| @@ -293,6 +383,15 @@ impl MemoryExec { | |||
| Boundedness::Bounded, | |||
| ) | |||
| } | |||
|
|
|||
| 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 |
|
🚀 |
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?