You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "AdamGS (via GitHub)" <gi...@apache.org> on 2025/02/28 23:35:04 UTC
[PR] Split out avro, parquet, json and csv into individual crates [datafusion]
AdamGS opened a new pull request, #14951:
URL: https://github.com/apache/datafusion/pull/14951
## Which issue does this PR close?
Part of #14444.
The PR is obviously huge, but because the file are split (keeping tests in core), github doesn't understand some of the changes as moves, even though that's the majority of the change.
I've read through it a couple of times and kept finding small things that can be improved and I'll probably do that again during the weekend.
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
Moving all format-specific functionality into dedicated crates, introducing:
- `datafusion-datasource-csv`
- `datafusion-datasource-json`
- `datafusion-datasource-avro`
- `datafusion-datasource-parquet`
Along the way I managed to move some stuff into `datasource` and tried multiple times to move tests infrastructure, but I think I ended pretty close to where I started.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
I tried to maintain the current exposed modules and types, and keep them sane in general.
I'm not sure I managed to keep all of them but I think none got significantly more complex to pull in.
There is more public API surface now, as some of the shared code used its location to stay `pub(crate)` or just private (for example - the `Decoder` trait and related pieces).
I also moved a couple of functions from `SessionState` to session, one tiny step towards that goal.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2695638528
`projected_file_column_names` only has one call site in Avro. I'm glad to add a deprecation notice from 47 and beyond and just put put the code there directly in the relevant place in `AvroSource`, just give me the +1.
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976486683
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
Review Comment:
My understanding is that since `Session` trait is implemented for `SessionState`, you should be able to call `some_SessionState_struct.default_table_options()`, It seems that you are essentially overriding that function with itself(hence the original suggestion of removing it) unless I am missing something. Please let me know if that's the case!!
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1978286601
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -501,7 +501,7 @@ impl FileScanConfig {
(schema, constraints, stats, output_ordering)
}
- #[cfg_attr(not(feature = "avro"), allow(unused))] // Only used by avro
Review Comment:
I think it is still in use (in avro-source `with_projection` function), just not detected
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976405859
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
Review Comment:
I think `default_table_options` and `table_options` only need to be part of `impl Session` rather than
`SessionState`
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -501,7 +501,7 @@ impl FileScanConfig {
(schema, constraints, stats, output_ordering)
}
- #[cfg_attr(not(feature = "avro"), allow(unused))] // Only used by avro
Review Comment:
It is my understanding that this is being removed to avoid dependency on avro.
I am not sure what is better between this and empty feature.
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
/// Return mutable table options
pub fn table_options_mut(&mut self) -> &mut TableOptions {
&mut self.table_options
Review Comment:
perhaps this should also be a part of Session
##########
datafusion/core/Cargo.toml:
##########
@@ -54,14 +60,15 @@ default = [
"string_expressions",
"unicode_expressions",
"compression",
+ "avro",
Review Comment:
should avro be a default feature?
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976494633
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
Review Comment:
I see, this seems like the proper way of doing things. Thanks for clarification!!
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2697208596
Thanks again @AdamGS and @logan-keede -- this is amazing progress. Something I have thought was important for the last t
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1978273718
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -501,7 +501,7 @@ impl FileScanConfig {
(schema, constraints, stats, output_ordering)
}
- #[cfg_attr(not(feature = "avro"), allow(unused))] // Only used by avro
Review Comment:
its a pub fn -- I think we should simply deprecate it if it isn't needed
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1979201859
##########
datafusion/core/src/datasource/file_format/avro.rs:
##########
@@ -15,163 +15,31 @@
// specific language governing permissions and limitations
// under the License.
-//! [`AvroFormat`] Apache Avro [`FileFormat`] abstractions
-
-use std::any::Any;
-use std::collections::HashMap;
-use std::fmt;
-use std::sync::Arc;
-
-use super::file_compression_type::FileCompressionType;
-use super::FileFormat;
-use super::FileFormatFactory;
-use crate::datasource::avro_to_arrow::read_avro_schema_from_reader;
-use crate::datasource::physical_plan::AvroSource;
-use crate::error::Result;
-use crate::physical_plan::ExecutionPlan;
-use crate::physical_plan::Statistics;
-
-use arrow::datatypes::Schema;
-use arrow::datatypes::SchemaRef;
-use async_trait::async_trait;
-use datafusion_catalog::Session;
-use datafusion_common::internal_err;
-use datafusion_common::parsers::CompressionTypeVariant;
-use datafusion_common::GetExt;
-use datafusion_common::DEFAULT_AVRO_EXTENSION;
-use datafusion_datasource::file::FileSource;
-use datafusion_datasource::file_scan_config::FileScanConfig;
-use datafusion_physical_expr::PhysicalExpr;
-use object_store::{GetResultPayload, ObjectMeta, ObjectStore};
-
-#[derive(Default)]
-/// Factory struct used to create [AvroFormat]
-pub struct AvroFormatFactory;
-
-impl AvroFormatFactory {
- /// Creates an instance of [AvroFormatFactory]
- pub fn new() -> Self {
- Self {}
- }
-}
-
-impl FileFormatFactory for AvroFormatFactory {
- fn create(
- &self,
- _state: &dyn Session,
- _format_options: &HashMap<String, String>,
- ) -> Result<Arc<dyn FileFormat>> {
- Ok(Arc::new(AvroFormat))
- }
+//! Re-exports the [`datafusion_datasource_avro::file_format`] module, and contains tests for it.
- fn default(&self) -> Arc<dyn FileFormat> {
- Arc::new(AvroFormat)
- }
-
- fn as_any(&self) -> &dyn Any {
- self
- }
-}
-
-impl fmt::Debug for AvroFormatFactory {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- f.debug_struct("AvroFormatFactory").finish()
- }
-}
-
-impl GetExt for AvroFormatFactory {
- fn get_ext(&self) -> String {
- // Removes the dot, i.e. ".parquet" -> "parquet"
- DEFAULT_AVRO_EXTENSION[1..].to_string()
- }
-}
-
-/// Avro `FileFormat` implementation.
-#[derive(Default, Debug)]
-pub struct AvroFormat;
-
-#[async_trait]
-impl FileFormat for AvroFormat {
- fn as_any(&self) -> &dyn Any {
- self
- }
-
- fn get_ext(&self) -> String {
- AvroFormatFactory::new().get_ext()
- }
-
- fn get_ext_with_compression(
- &self,
- file_compression_type: &FileCompressionType,
- ) -> Result<String> {
- let ext = self.get_ext();
- match file_compression_type.get_variant() {
- CompressionTypeVariant::UNCOMPRESSED => Ok(ext),
- _ => internal_err!("Avro FileFormat does not support compression."),
- }
- }
-
- async fn infer_schema(
- &self,
- _state: &dyn Session,
- store: &Arc<dyn ObjectStore>,
- objects: &[ObjectMeta],
- ) -> Result<SchemaRef> {
- let mut schemas = vec![];
- for object in objects {
- let r = store.as_ref().get(&object.location).await?;
- let schema = match r.payload {
- GetResultPayload::File(mut file, _) => {
- read_avro_schema_from_reader(&mut file)?
- }
- GetResultPayload::Stream(_) => {
- // TODO: Fetching entire file to get schema is potentially wasteful
- let data = r.bytes().await?;
- read_avro_schema_from_reader(&mut data.as_ref())?
- }
- };
- schemas.push(schema);
- }
- let merged_schema = Schema::try_merge(schemas)?;
- Ok(Arc::new(merged_schema))
- }
-
- async fn infer_stats(
- &self,
- _state: &dyn Session,
- _store: &Arc<dyn ObjectStore>,
- table_schema: SchemaRef,
- _object: &ObjectMeta,
- ) -> Result<Statistics> {
- Ok(Statistics::new_unknown(&table_schema))
- }
-
- async fn create_physical_plan(
- &self,
- _state: &dyn Session,
- conf: FileScanConfig,
- _filters: Option<&Arc<dyn PhysicalExpr>>,
- ) -> Result<Arc<dyn ExecutionPlan>> {
- Ok(conf.with_source(self.file_source()).build())
- }
-
- fn file_source(&self) -> Arc<dyn FileSource> {
- Arc::new(AvroSource::new())
- }
-}
+pub use datafusion_datasource_avro::file_format::*;
#[cfg(test)]
-#[cfg(feature = "avro")]
mod tests {
Review Comment:
Oh I didn't realize that was a thing. Glad to move everything there (in this PR or a different one).
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2695770658
After some more thinking - while I couldn't find any use of this function in an active codebase on github, I can see why its useful (or at least nice). I just removed the `unused` on it, I don't see a real reason to deprecate it unless there's some bigger re-thinking of the API here.
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1978286601
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -501,7 +501,7 @@ impl FileScanConfig {
(schema, constraints, stats, output_ordering)
}
- #[cfg_attr(not(feature = "avro"), allow(unused))] // Only used by avro
Review Comment:
I think it is still in use (in avro-source with_projection`), just not detected
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2695644970
> `projected_file_column_names` only has one call site in Avro. I'm glad to add a deprecation notice from 47 and beyond and just put put the code there directly in the relevant place in `AvroSource`, just give me the +1.
That would be nice.
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976481579
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
Review Comment:
My only concern is that might be a breaking change for some users, but its probably very minor one in the scope of this PR and other changes
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2695764126
For what its worth - the only non-datafusion usage of this function I could find on github is in a [project](https://github.com/datafusion-contrib/datafusion-tokomak) that wasn't active for over three years.
I'll push a commit with a deprecation + created a copy of the function for tests and gave `AvroSource` its own site, that way we can delete it whenever, not sure what the deprecation policy is, @alamb obviously feel free to drop that commit if you think its unnecessary.
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2697209311
I am merging this one in so it doesn't accumulate conflicts
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976481607
##########
datafusion/core/Cargo.toml:
##########
@@ -54,14 +60,15 @@ default = [
"string_expressions",
"unicode_expressions",
"compression",
+ "avro",
Review Comment:
probably not, I'll remove 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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976488608
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
Review Comment:
If I change the function here to:
```rust
/// return the TableOptions options with its extensions
pub fn default_table_options(&self) -> TableOptions {
self.default_table_options()
}
```
clippy tells me its a recursion and the stack blows up when I run tests.
I think the default implementation here messes with with just having the trait implementation delegate to the struct. Of course I can copy it here and make the implementation delegate to it but that seems like unnecessary code duplication.
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976484614
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
Review Comment:
reread the code around it, and seems like the `Session` implementation mostly delegates to the actual implementation, which saves bringing it into scope in a ton of locations across the codebase.
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2695612573
Thank you @logan-keede for your thoughts and suggestions
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2692720801
Thank you -- I will look at this shortly
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1978271600
##########
datafusion/core/src/datasource/file_format/avro.rs:
##########
@@ -15,163 +15,31 @@
// specific language governing permissions and limitations
// under the License.
-//! [`AvroFormat`] Apache Avro [`FileFormat`] abstractions
-
-use std::any::Any;
-use std::collections::HashMap;
-use std::fmt;
-use std::sync::Arc;
-
-use super::file_compression_type::FileCompressionType;
-use super::FileFormat;
-use super::FileFormatFactory;
-use crate::datasource::avro_to_arrow::read_avro_schema_from_reader;
-use crate::datasource::physical_plan::AvroSource;
-use crate::error::Result;
-use crate::physical_plan::ExecutionPlan;
-use crate::physical_plan::Statistics;
-
-use arrow::datatypes::Schema;
-use arrow::datatypes::SchemaRef;
-use async_trait::async_trait;
-use datafusion_catalog::Session;
-use datafusion_common::internal_err;
-use datafusion_common::parsers::CompressionTypeVariant;
-use datafusion_common::GetExt;
-use datafusion_common::DEFAULT_AVRO_EXTENSION;
-use datafusion_datasource::file::FileSource;
-use datafusion_datasource::file_scan_config::FileScanConfig;
-use datafusion_physical_expr::PhysicalExpr;
-use object_store::{GetResultPayload, ObjectMeta, ObjectStore};
-
-#[derive(Default)]
-/// Factory struct used to create [AvroFormat]
-pub struct AvroFormatFactory;
-
-impl AvroFormatFactory {
- /// Creates an instance of [AvroFormatFactory]
- pub fn new() -> Self {
- Self {}
- }
-}
-
-impl FileFormatFactory for AvroFormatFactory {
- fn create(
- &self,
- _state: &dyn Session,
- _format_options: &HashMap<String, String>,
- ) -> Result<Arc<dyn FileFormat>> {
- Ok(Arc::new(AvroFormat))
- }
+//! Re-exports the [`datafusion_datasource_avro::file_format`] module, and contains tests for it.
- fn default(&self) -> Arc<dyn FileFormat> {
- Arc::new(AvroFormat)
- }
-
- fn as_any(&self) -> &dyn Any {
- self
- }
-}
-
-impl fmt::Debug for AvroFormatFactory {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- f.debug_struct("AvroFormatFactory").finish()
- }
-}
-
-impl GetExt for AvroFormatFactory {
- fn get_ext(&self) -> String {
- // Removes the dot, i.e. ".parquet" -> "parquet"
- DEFAULT_AVRO_EXTENSION[1..].to_string()
- }
-}
-
-/// Avro `FileFormat` implementation.
-#[derive(Default, Debug)]
-pub struct AvroFormat;
-
-#[async_trait]
-impl FileFormat for AvroFormat {
- fn as_any(&self) -> &dyn Any {
- self
- }
-
- fn get_ext(&self) -> String {
- AvroFormatFactory::new().get_ext()
- }
-
- fn get_ext_with_compression(
- &self,
- file_compression_type: &FileCompressionType,
- ) -> Result<String> {
- let ext = self.get_ext();
- match file_compression_type.get_variant() {
- CompressionTypeVariant::UNCOMPRESSED => Ok(ext),
- _ => internal_err!("Avro FileFormat does not support compression."),
- }
- }
-
- async fn infer_schema(
- &self,
- _state: &dyn Session,
- store: &Arc<dyn ObjectStore>,
- objects: &[ObjectMeta],
- ) -> Result<SchemaRef> {
- let mut schemas = vec![];
- for object in objects {
- let r = store.as_ref().get(&object.location).await?;
- let schema = match r.payload {
- GetResultPayload::File(mut file, _) => {
- read_avro_schema_from_reader(&mut file)?
- }
- GetResultPayload::Stream(_) => {
- // TODO: Fetching entire file to get schema is potentially wasteful
- let data = r.bytes().await?;
- read_avro_schema_from_reader(&mut data.as_ref())?
- }
- };
- schemas.push(schema);
- }
- let merged_schema = Schema::try_merge(schemas)?;
- Ok(Arc::new(merged_schema))
- }
-
- async fn infer_stats(
- &self,
- _state: &dyn Session,
- _store: &Arc<dyn ObjectStore>,
- table_schema: SchemaRef,
- _object: &ObjectMeta,
- ) -> Result<Statistics> {
- Ok(Statistics::new_unknown(&table_schema))
- }
-
- async fn create_physical_plan(
- &self,
- _state: &dyn Session,
- conf: FileScanConfig,
- _filters: Option<&Arc<dyn PhysicalExpr>>,
- ) -> Result<Arc<dyn ExecutionPlan>> {
- Ok(conf.with_source(self.file_source()).build())
- }
-
- fn file_source(&self) -> Arc<dyn FileSource> {
- Arc::new(AvroSource::new())
- }
-}
+pub use datafusion_datasource_avro::file_format::*;
#[cfg(test)]
-#[cfg(feature = "avro")]
mod tests {
Review Comment:
it might be nice (eventually) to move these tests to core_integration somewhere
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#issuecomment-2695644151
actually - given that the avro feature is completly gone from `datafusion-datasource` I think we can just delete it here, the logican can be a private function in `datasource-avro/src/source.rs`.
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #14951:
URL: https://github.com/apache/datafusion/pull/14951
--
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: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976406045
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
self.config.options()
}
- /// return the TableOptions options with its extensions
- pub fn default_table_options(&self) -> TableOptions {
- self.table_options
- .combine_with_session_config(self.config_options())
- }
-
/// Return the table options
pub fn table_options(&self) -> &TableOptions {
&self.table_options
}
+ /// return the TableOptions options with its extensions
+ pub fn default_table_options(&self) -> TableOptions {
+ Session::default_table_options(self)
+ }
+
/// Return mutable table options
pub fn table_options_mut(&mut self) -> &mut TableOptions {
&mut self.table_options
Review Comment:
perhaps `table_options_mut` should also be a part of Session
--
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