You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mach-kernel (via GitHub)" <gi...@apache.org> on 2025/10/08 12:34:03 UTC
[PR] FileScanConfig: Preserve schema metadata across ser/de boundary [datafusion]
mach-kernel opened a new pull request, #17966:
URL: https://github.com/apache/datafusion/pull/17966
## Which issue does this PR close?
n/a
- Closes #.
## Rationale for this change
When serializing `DataSourceExec` plans with a `FileScanConfig`, any metadata that is decorated to the schema is expected to be there after ser/de
## What changes are included in this PR?
- Schema metadata is explicitly handled in to_proto/from_proto
- A test is added that fails on `main`
## Are these changes tested?
Yes
## Are there any user-facing changes?
No
--
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] FileScanConfig: Preserve schema metadata across ser/de boundary [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #17966:
URL: https://github.com/apache/datafusion/pull/17966#issuecomment-3381544245
one other question for you @mach-kernel as you posted this in ballista group, do you need this to get to ballista 50? if so maybe we could back-port to datafusion 50.2 if @xudong963 agrees
--
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] FileScanConfig: Preserve schema metadata across ser/de boundary [datafusion]
Posted by "mach-kernel (via GitHub)" <gi...@apache.org>.
mach-kernel commented on PR #17966:
URL: https://github.com/apache/datafusion/pull/17966#issuecomment-3382499011
> one other question for you @mach-kernel as you posted this in ballista group, do you need this to get to ballista 50? if so maybe we could back-port to datafusion 50.2 if @xudong963 agrees
We can wait -- though thank you for asking @milenkovicm!
--
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] FileScanConfig: Preserve schema metadata across ser/de boundary [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #17966:
URL: https://github.com/apache/datafusion/pull/17966#discussion_r2413849310
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -2221,3 +2224,41 @@ async fn roundtrip_memory_source() -> Result<()> {
.await?;
roundtrip_test(plan)
}
+
+#[tokio::test]
+async fn roundtrip_listing_table_with_schema_metadata() -> Result<()> {
+ let ctx = SessionContext::new();
+ let file_format = JsonFormat::default();
+ let table_partition_cols = vec![("part".to_owned(), DataType::Int64)];
+ let data = "../core/tests/data/partitioned_table_json";
+ let listing_table_url = ListingTableUrl::parse(data)?;
+ let listing_options = ListingOptions::new(Arc::new(file_format))
+ .with_table_partition_cols(table_partition_cols);
+
+ let config = ListingTableConfig::new(listing_table_url)
+ .with_listing_options(listing_options)
+ .infer_schema(&ctx.state())
+ .await?;
+
+ // Decorate metadata onto the inferred ListingTable schema
+ let schema_with_meta = config
+ .file_schema
+ .clone()
+ .map(|s| {
+ let mut meta: HashMap<String, String> = HashMap::new();
+ meta.insert("foo.bar".to_string(), "baz".to_string());
+ s.as_ref().clone().with_metadata(meta)
+ })
+ .expect("Must decorate metadata");
+
+ let config = config.with_schema(Arc::new(schema_with_meta));
+ ctx.register_table("hive_style", Arc::new(ListingTable::try_new(config)?))?;
+
+ let plan = ctx
+ .sql("select * from hive_style limit 1")
+ .await?
+ .create_physical_plan()
+ .await?;
+
+ roundtrip_test(plan)
Review Comment:
Roundtrip test will compare string representation of the plan, which may or may not print metadata (i'm not sure). Would it make sense to test `parse_protobuf_file_scan_config(serialize_file_scan_config)` and check if configs match ?
--
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] FileScanConfig: Preserve schema metadata across ser/de boundary [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #17966:
URL: https://github.com/apache/datafusion/pull/17966
--
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] FileScanConfig: Preserve schema metadata across ser/de boundary [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #17966:
URL: https://github.com/apache/datafusion/pull/17966#discussion_r2414294432
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -2221,3 +2224,41 @@ async fn roundtrip_memory_source() -> Result<()> {
.await?;
roundtrip_test(plan)
}
+
+#[tokio::test]
+async fn roundtrip_listing_table_with_schema_metadata() -> Result<()> {
+ let ctx = SessionContext::new();
+ let file_format = JsonFormat::default();
+ let table_partition_cols = vec![("part".to_owned(), DataType::Int64)];
+ let data = "../core/tests/data/partitioned_table_json";
+ let listing_table_url = ListingTableUrl::parse(data)?;
+ let listing_options = ListingOptions::new(Arc::new(file_format))
+ .with_table_partition_cols(table_partition_cols);
+
+ let config = ListingTableConfig::new(listing_table_url)
+ .with_listing_options(listing_options)
+ .infer_schema(&ctx.state())
+ .await?;
+
+ // Decorate metadata onto the inferred ListingTable schema
+ let schema_with_meta = config
+ .file_schema
+ .clone()
+ .map(|s| {
+ let mut meta: HashMap<String, String> = HashMap::new();
+ meta.insert("foo.bar".to_string(), "baz".to_string());
+ s.as_ref().clone().with_metadata(meta)
+ })
+ .expect("Must decorate metadata");
+
+ let config = config.with_schema(Arc::new(schema_with_meta));
+ ctx.register_table("hive_style", Arc::new(ListingTable::try_new(config)?))?;
+
+ let plan = ctx
+ .sql("select * from hive_style limit 1")
+ .await?
+ .create_physical_plan()
+ .await?;
+
+ roundtrip_test(plan)
Review Comment:
retracting my comment, looks like others are tested in similar way
--
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