You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "berkaysynnada (via GitHub)" <gi...@apache.org> on 2024/12/19 10:32:42 UTC
[I] Single Source Exec Across All Providers [datafusion]
berkaysynnada opened a new issue, #13838:
URL: https://github.com/apache/datafusion/issues/13838
### Is your feature request related to a problem or challenge?
I would like to revive https://github.com/apache/datafusion/issues/6339, and specially https://github.com/apache/datafusion/issues/6339#issuecomment-1546521450. While doing our fork specific implementations, we mostly think that there should be a single exec for sources, just like sinks. We've been coming across this idea a lot lately. I'd like to ask the community opinion, and to know any against idea or counter argument for this. I know it will requires much effort if it is approved, but we can possibly work on that.
### Describe the solution you'd like
TableProvider's scan()'s the same Exec for the all file formats.
### Describe alternatives you've considered
_No response_
### Additional context
_No response_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2553691937
What example problems would this solve?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2553778873
> Is it easy to explain some of these scenarios?
> > In the simplest case, when we want to add a method to the ExecutionPlan API that applies uniformly to all sources, we have to repeat its implementation for each source.
This comes up very often and hinders extensibility. Example situations we ran into includes things like checkpointing support, watermark generation/handling, etc. Almost none of these things (and neither other functionality that is already in upstream DataFusion) have anything to do with source operator reading a CSV or a JSON, but somehow we have separate operators like `CSVExec`, `JsonExec` etc.
> I think this would be a pretty major breaking change for all downstream consumers.
Indeed. That's why we wanted to discuss and see how we can approach this as a community.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2553488642
(I also changed the title of this ticket to refer to the struct names I think is being suggested, please let me know / correct it if I got that incorrect)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2553535567
> Is it easy to explain some of these scenarios?
In the simplest case, when we want to add a method to the ExecutionPlan API that applies uniformly to all sources, we have to repeat its implementation for each source. Simply checking the children count doesn’t always resolve the issue. Having another API, like is_source(), and consult that, but even this doesn’t result in perfect usage.
The asymmetric sink-source pattern also feels somewhat unusual.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2554248556
If we have uniform operator on top of CSV or JSON, it will internally dispatch to a reader (CSV or JSON, etc.) but won't have any file-format (or datasource-) -specific logic.
If we don't have uniform operator, we still can add checkpointing no top of it with an additional operator sitting above CSVExec, JsonExec.
Thus at the first sight the two approaches look equally expressive, which means I am missing some important detail. What is it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak closed issue #13838: [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders`
URL: https://github.com/apache/datafusion/issues/13838
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2554324804
> Thus at the first sight the two approaches look equally expressive, which means I am missing some important detail. What is it?
This is actually what we do in our fork. They are indeed equally expressive, but this starts to become a problem as we start talking about more features; i.e. watermarks, out-of-order handling etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2554943551
> We are lucky to be doing engineering in a field where most problems of this type has workarounds and solutions, but when they start piling on, IMO it is a good signal that some lower-level design was wrong. In this case, it seems like that is the non-uniformity on the source side.
So my "gut" feeling is that this change would basically push complexity around (make implementing `TableProviders` outside the DataFusion core more complicated) but I don't think I have much more than that opinion to share
One potential way to proceed with this idea would be to sketch out what this idea would look like in a PR and try to adapt some existing open source table providers and see the impact
These are some obvious candidates:
- https://github.com/delta-io/delta-rs
- https://github.com/datafusion-contrib/datafusion-table-providers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [DISCUSS] Single Source `ExecutionPlan` Across All `TableProviders` [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2555026849
I think this is a great idea. We can see the impact clearly on both DF-core sources and external ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Single Source Exec Across All Providers [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2553413003
cc @alamb, @andygrove, @Dandandan, @jayzhan-synnada
Also @findepi it'd be great if you can also chime in
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Single Source Exec Across All Providers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13838:
URL: https://github.com/apache/datafusion/issues/13838#issuecomment-2553486646
> TableProvider's scan()'s the same Exec for the all file formats
I think this would be a pretty major breaking change for all downstream consumers.
> we have frequently encountered scenarios where it seems more appropriate to have a single exec for sources, similar to the approach used for sinks. This idea has been coming up a lot recently.
Is it easy to explain some of these scenarios?
Rather than trying to use the same ExecutionPlan for all TableProviders, another thing to do might be to extend the `ExecutionPlan` trait with the common functionality.
For example instead of having to do something like
```rust
if let Some(parquet_exec) = plan.as_any().downcast_ref::<Parquetexec>() {
parquet_specific_function(parquet_exec)
}
```
The other thing we could do is
```rust
trait ExecutionPlan {
// do specific thing
fn specific_function(&self) {}
}
```
And then the pass / analysis would be able to use do
```rust
plan.specific_function()
```
I may be jumping to conclusions however and misguess what you are trying to do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org