You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Acfboy (via GitHub)" <gi...@apache.org> on 2026/03/02 02:53:33 UTC
[PR] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Acfboy opened a new pull request, #20638:
URL: https://github.com/apache/datafusion/pull/20638
…d in file formats
## Which issue does this PR close?
<!--
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.
-->
- Closes #16944.
## Rationale for this change
Currently, the `LogicalExtensionCodec` implementation for `DefaultLogicalExtensionCodec` leaves `try_decode_file_format` / `try_encode_file_format` unimplemented (returning "not implemented" errors). However, the actual serialization logic for built-in file formats — arrow, parquet, csv, and json— already exists in their respective codec implementations. All we need to do is tag which format is being used, and delegate to the corresponding format-specific codec to handle the data.
<!--
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?
Added a `FileFormatKind` enum and a `FileFormatProto` message to `datafusion.proto` to identify the file format type during transmission. Implemented `try_decode_file_format` and `try_encode_file_format` for `DefaultLogicalExtensionCodec`, which dispatch serialization/deserialization to the corresponding format-specific codec based on the format kind. Note that Avro is not covered because the upstream repository has not yet implemented the corresponding Avro codec, so Avro support is not functional at this time.
## Are these changes tested?
Yes. Roundtrip tests are included for csv, json, parquet, and arrow.
<!--
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?
<!--
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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "Acfboy (via GitHub)" <gi...@apache.org>.
Acfboy commented on code in PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#discussion_r2896051453
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
+
+ let any = node.as_any();
Review Comment:
thx 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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "Acfboy (via GitHub)" <gi...@apache.org>.
Acfboy commented on code in PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#discussion_r2894113183
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
Review Comment:
Sorry, it’s my bad habit carried over from python. I'll fix 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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#discussion_r2892071579
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
Review Comment:
should we put imports on top of the file?
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
Review Comment:
should we put imports on top of the file?
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
Review Comment:
would it make sense to have to add it on top of the file
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
+
+ let any = node.as_any();
+ let mut options = Vec::new();
Review Comment:
its encoded file format, not an option
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -270,6 +270,20 @@ message CopyToNode {
repeated string partition_by = 7;
}
+enum FileFormatKind {
+ FILE_FORMAT_KIND_UNSPECIFIED = 0;
+ FILE_FORMAT_KIND_CSV = 1;
+ FILE_FORMAT_KIND_JSON = 2;
+ FILE_FORMAT_KIND_PARQUET = 3;
+ FILE_FORMAT_KIND_ARROW = 4;
+ FILE_FORMAT_KIND_AVRO = 5;
+}
+
Review Comment:
could we add some comments desribing what this message does
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
Review Comment:
same here, is there a reason why imports can't go at file beginning
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
+
+ let any = node.as_any();
Review Comment:
do we need to extract variable here?
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
+
+ let any = node.as_any();
Review Comment:
do we need to extract it as a variable here
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
+
+ let any = node.as_any();
+ let mut options = Vec::new();
Review Comment:
maybe some other name than `options`
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -270,6 +270,20 @@ message CopyToNode {
repeated string partition_by = 7;
}
+enum FileFormatKind {
+ FILE_FORMAT_KIND_UNSPECIFIED = 0;
+ FILE_FORMAT_KIND_CSV = 1;
+ FILE_FORMAT_KIND_JSON = 2;
+ FILE_FORMAT_KIND_PARQUET = 3;
+ FILE_FORMAT_KIND_ARROW = 4;
+ FILE_FORMAT_KIND_AVRO = 5;
+}
+
+message FileFormatProto {
+ FileFormatKind kind = 1;
+ bytes options = 2;
Review Comment:
its not `option` its encoded file format, maybe more informative name
--
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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "Acfboy (via GitHub)" <gi...@apache.org>.
Acfboy commented on PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#issuecomment-3997720153
@milenkovicm Looking forward to your advice!
--
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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#issuecomment-4003034594
Thanks @Acfboy will have a look today
--
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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "Acfboy (via GitHub)" <gi...@apache.org>.
Acfboy commented on code in PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#discussion_r2894095704
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -270,6 +270,20 @@ message CopyToNode {
repeated string partition_by = 7;
}
+enum FileFormatKind {
+ FILE_FORMAT_KIND_UNSPECIFIED = 0;
+ FILE_FORMAT_KIND_CSV = 1;
+ FILE_FORMAT_KIND_JSON = 2;
+ FILE_FORMAT_KIND_PARQUET = 3;
+ FILE_FORMAT_KIND_ARROW = 4;
+ FILE_FORMAT_KIND_AVRO = 5;
+}
+
+message FileFormatProto {
+ FileFormatKind kind = 1;
+ bytes options = 2;
Review Comment:
Thanks! That was my mistake. I used `options` because I was thinking of it as 'file format options'. I agree a more descriptive name would be better.
--
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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm merged PR #20638:
URL: https://github.com/apache/datafusion/pull/20638
--
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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "Acfboy (via GitHub)" <gi...@apache.org>.
Acfboy commented on code in PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#discussion_r2894089591
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
+
+ let any = node.as_any();
Review Comment:
Do you mean we should inline `node.as_any()` instead of binding it to `let any`? I don't quite follow.
--
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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "Acfboy (via GitHub)" <gi...@apache.org>.
Acfboy commented on code in PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#discussion_r2894113183
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
Review Comment:
Sorry, I guess it’s my bad habit carried over from python. I'll fix 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] feat: make DefaultLogicalExtensionCodec support serialisation of buil… [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #20638:
URL: https://github.com/apache/datafusion/pull/20638#discussion_r2894728452
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -207,6 +207,102 @@ impl LogicalExtensionCodec for DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_file_format(
+ &self,
+ buf: &[u8],
+ ctx: &TaskContext,
+ ) -> Result<Arc<dyn FileFormatFactory>> {
+ use prost::Message;
+
+ let proto = protobuf::FileFormatProto::decode(buf).map_err(|e| {
+ internal_datafusion_err!("Failed to decode FileFormatProto: {e}")
+ })?;
+
+ let kind = protobuf::FileFormatKind::try_from(proto.kind).map_err(|_| {
+ internal_datafusion_err!("Unknown FileFormatKind: {}", proto.kind)
+ })?;
+
+ match kind {
+ protobuf::FileFormatKind::Csv => file_formats::CsvLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Json => file_formats::JsonLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(feature = "parquet")]
+ protobuf::FileFormatKind::Parquet => {
+ file_formats::ParquetLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx)
+ }
+ protobuf::FileFormatKind::Arrow => file_formats::ArrowLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ protobuf::FileFormatKind::Avro => file_formats::AvroLogicalExtensionCodec
+ .try_decode_file_format(&proto.options, ctx),
+ #[cfg(not(feature = "parquet"))]
+ protobuf::FileFormatKind::Parquet => {
+ not_impl_err!("Parquet support requires the 'parquet' feature")
+ }
+ protobuf::FileFormatKind::Unspecified => {
+ not_impl_err!("Unspecified file format kind")
+ }
+ }
+ }
+
+ fn try_encode_file_format(
+ &self,
+ buf: &mut Vec<u8>,
+ node: Arc<dyn FileFormatFactory>,
+ ) -> Result<()> {
+ use datafusion_datasource_arrow::file_format::ArrowFormatFactory;
+ use datafusion_datasource_csv::file_format::CsvFormatFactory;
+ use datafusion_datasource_json::file_format::JsonFormatFactory;
+ use prost::Message;
+
+ let any = node.as_any();
Review Comment:
yes, sorry for confusion
--
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