You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In that case, it's not really empty. This is confusing and a source of bugs because optimization passes now have to check if that flag is set or not (e.g. to prune sub-trees).
I would also argue that the use case of "produce row" is different: while EmptyExec can totally occur during production (e.g. when data / sub-trees are pruned based on statistics) the "one row" case is very likely for testing.
I think this is a reasonable first (maybe more like second) issue -- it is fairly mechanical but will require figuring out some of how ExecutionPlan works and updating a bunch of tests
I have tried to move the produce_one_row to MemoryExec in #8412, but it wasn't straight forward as there are no try_into_physical_plan and try_from_physical_plan implemented for MemoryExec.
Anyway I have tried to implement the physical plan, but since MemoryExec contains dyn Array for storing it's partitions it's a little bit tricky to serialize it to proto, the best way coming to my mind is to json serialize it as a string field in proto.
I can do that, but before I wanted to get feedback here and make sure if that's a good path to go? @alamb@tustvold@crepererum
I have tried to move the produce_one_row to MemoryExec in #8412, but it wasn't straight forward as there are no try_into_physical_plan and try_from_physical_plan implemented for MemoryExec.
Anyway I have tried to implement the physical plan, but since MemoryExec contains dyn Array for storing it's partitions it's a little bit tricky to serialize it to proto, the best way coming to my mind is to json serialize it as a string field in proto.
I can do that, but before I wanted to get feedback here and make sure if that's a good path to go? @alamb@tustvold@crepererum
The serialization is a good point
What about perhaps making a new OneRowExec instead of reusing MemoryExec ? I think the code would be fairly straightforward.
Activity
alamb commentedon Nov 29, 2023
I think this is a reasonable first (maybe more like second) issue -- it is fairly mechanical but will require figuring out some of how ExecutionPlan works and updating a bunch of tests
[-]Split `EmptyExec`[/-][+]Split `EmptyExec` into `OneRowExec`[/+]tustvold commentedon Nov 29, 2023
FWIW unless the single-row aspect of OneRowExec is important, it could possibly just use MemoryExec - https://docs.rs/datafusion/latest/datafusion/physical_plan/memory/struct.MemoryExec.html
alamb commentedon Nov 29, 2023
single_rowis used for queries likeselect now()I think we can now make a batch now with no columns but 1 row via https://docs.rs/arrow/latest/arrow/array/struct.RecordBatch.html#method.try_new_with_options which was not always the case . Using
MemoryExecmight work just fine.razeghi71 commentedon Dec 6, 2023
I have tried to move the
produce_one_rowtoMemoryExecin #8412, but it wasn't straight forward as there are notry_into_physical_planandtry_from_physical_planimplemented forMemoryExec.Anyway I have tried to implement the physical plan, but since
MemoryExeccontainsdyn Arrayfor storing it's partitions it's a little bit tricky to serialize it to proto, the best way coming to my mind is to json serialize it as a string field in proto.I can do that, but before I wanted to get feedback here and make sure if that's a good path to go? @alamb @tustvold @crepererum
alamb commentedon Dec 6, 2023
The serialization is a good point
What about perhaps making a new
OneRowExecinstead of reusingMemoryExec? I think the code would be fairly straightforward.