You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/15 11:15:31 UTC
[GitHub] [arrow-datafusion] doki23 opened a new pull request, #4221: Nonstring partitioned cols
doki23 opened a new pull request, #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221
# Which issue does this PR close?
Closes #4218.
# Rationale for this change
For supporting more types of partitioned columns.
# What changes are included in this PR?
This pr change datafusion to extract data type of partitioned columns from file groups instead of setting a default `Utf8` type.
# Are these changes tested?
Already passed all unit tests including many *_with_partitions
# Are there any user-facing changes?
Yes. Users can query partitioned columns by delta table definition.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#discussion_r1029481591
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -88,17 +89,45 @@ impl TableProviderFactory for ListingTableFactory {
),
};
- let provided_schema = if cmd.schema.fields().is_empty() {
- None
+ let (provided_schema, table_partition_cols) = if cmd.schema.fields().is_empty() {
+ (
+ None,
+ cmd.table_partition_cols
+ .iter()
+ .zip((0..cmd.table_partition_cols.len()).map(|_| DataType::Utf8))
+ .map(|x| (x.0.clone(), x.1))
+ .collect::<Vec<_>>(),
+ )
} else {
- Some(Arc::new(cmd.schema.as_ref().to_owned().into()))
+ let schema: SchemaRef = Arc::new(cmd.schema.as_ref().to_owned().into());
+ let table_partition_cols = cmd
+ .table_partition_cols
+ .iter()
+ .map(|col| {
+ (
+ col.clone(),
+ schema.field_with_name(col).unwrap().data_type().clone(),
Review Comment:
Are you sure it is ok to `unwrap()` here (which will panic)? Given the function returns a `Result` perhaps we could return an error if the field was not present in the schema.
##########
datafusion/proto/src/logical_plan.rs:
##########
@@ -876,7 +890,7 @@ impl AsLogicalPlan for LogicalPlanNode {
FileFormatType::Avro(protobuf::AvroFormat {})
} else {
return Err(proto_error(format!(
- "Error converting file format, {:?} is invalid as a datafusion foramt.",
+ "Error converting file format, {:?} is invalid as a datafusion format.",
Review Comment:
👍
##########
datafusion/core/tests/path_partition.rs:
##########
@@ -56,7 +57,11 @@ async fn parquet_distinct_partition_col() -> Result<()> {
"year=2021/month=10/day=09/file.parquet",
"year=2021/month=10/day=28/file.parquet",
],
- &["year", "month", "day"],
+ &[
+ ("year", DataType::Int32),
Review Comment:
Nice
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -88,17 +89,45 @@ impl TableProviderFactory for ListingTableFactory {
),
};
- let provided_schema = if cmd.schema.fields().is_empty() {
- None
+ let (provided_schema, table_partition_cols) = if cmd.schema.fields().is_empty() {
+ (
+ None,
+ cmd.table_partition_cols
+ .iter()
+ .zip((0..cmd.table_partition_cols.len()).map(|_| DataType::Utf8))
+ .map(|x| (x.0.clone(), x.1))
Review Comment:
```suggestion
.map(|x| (x.clone(), DataType::Utf8))
```
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -270,16 +268,19 @@ impl ListingOptions {
///
/// ```
/// use std::sync::Arc;
+ /// use arrow::datatypes::DataType;
/// use datafusion::datasource::{listing::ListingOptions, file_format::parquet::ParquetFormat};
///
/// let listing_options = ListingOptions::new(Arc::new(ParquetFormat::default()))
- /// .with_table_partition_cols(vec!["col_a".to_string(), "col_b".to_string()]);
+ /// .with_table_partition_cols(vec![("col_a".to_string(), DataType::Utf8),
+ /// ("col_b".to_string(), DataType::Utf8)]);
///
- /// assert_eq!(listing_options.table_partition_cols, vec!["col_a", "col_b"]);
+ /// assert_eq!(listing_options.table_partition_cols, vec![("col_a".to_string(), DataType::Utf8),
+ /// ("col_b".to_string(), DataType::Utf8)]);
/// ```
pub fn with_table_partition_cols(
mut self,
- table_partition_cols: Vec<String>,
+ table_partition_cols: Vec<(String, DataType)>,
Review Comment:
It would help me read the code if this was a named struct so the code could refer to `.name` and `.datatype` rather than `.0` and `.1`. But I don't think it is necessary to merge this PR
```rust
#[derive(Clone)]
struct PartitionColumn {
name: String,
data_type: DataType
}
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#discussion_r1030743334
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -536,6 +537,23 @@ impl TableProvider for ListingTable {
return Ok(Arc::new(EmptyExec::new(false, projected_schema)));
}
+ // extract types of partition columns
+ let table_partition_cols = self
+ .options
+ .table_partition_cols
+ .iter()
+ .map(|col| {
+ (
+ col.0.to_owned(),
+ self.table_schema
+ .field_with_name(&col.0)
+ .unwrap()
Review Comment:
I think this is going to `panic` if the user specifies a partition column that is not present.
It would be nice to make it an error -- can you either do so as a follow on PR or file a ticket?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #4221: Support types other than String for partition columns on ListingTables
Posted by GitBox <gi...@apache.org>.
alamb merged PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] doki23 commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
doki23 commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1321770817
@mingmwang @alamb Would you please review my pr? I think it's ready now.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
mingmwang commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1315530960
I think not all the types of columns can be used as partition columns, should there a white list for supported types?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #4221: Support types other than String for partition columns on ListingTables
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1325462383
Benchmark runs are scheduled for baseline = 502b7e3cae169b25caf85062be6dfca32f9f173b and contender = 55bf8e9c2ca6d358e7dc4c37e1439de38008f960. 55bf8e9c2ca6d358e7dc4c37e1439de38008f960 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/0060c9e5d5634e0e90a8f7455c62a9ce...bd6a149aee8f4b0bbfe0d5c3f954f8e1/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/8e1fbd4228034ded9cbca9210f5472d6...17dac7d9674d4e2fa1893e733375aef3/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ba8711e485514f098f17e10d42b50374...d467bd0b100a42308e8530c4c877fc9a/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/eb3bc4dec2274a5993bdf7190fa7e15b...9ea7c1f57552426a9f1faa48e33f9167/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] doki23 commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
doki23 commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1323875308
@alamb Thank you. I'll apply more changes according to your suggestions tomorrow to make it 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] doki23 commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
doki23 commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1316385432
I find that [ListingTable](https://github.com/apache/arrow-datafusion/blob/406c1087bc16f8d2a49e5a9b05d2a0e1b67f7aa5/datafusion/core/src/datasource/listing/table.rs#L416) also treats type of partitioned columns as `Utf8`. I plan to change the struct [ListOptions](https://github.com/apache/arrow-datafusion/blob/406c1087bc16f8d2a49e5a9b05d2a0e1b67f7aa5/datafusion/core/src/datasource/listing/table.rs#L207).
There 2 ways:
1. change the existing property `table_partition_cols` but it's a break change to proto.
2. add an optional property `table_partition_fields` whose type is `Option<Vec<Field>>`.
I am appreciative if any 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] doki23 commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
doki23 commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1316161354
> I think not all the types of columns can be used as partition columns, should there a white list for supported types?
> And can you explain a little how you will infer the partition column types or partition spec from the file paths ? And what happened if there are conflicts?
Thanks for your reply. I believe the answers are same -- these types are provided by every datasource itself if I am not mistaken. So what I do is to make them matches to each other by their definitions.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
mingmwang commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1315544941
And can you explain a little how you will infer the partition column types or partition spec from the file paths ?
And what happened if there are 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1315784156
Thank you @doki23 -- I think the functionality needs some tests before we would consider merging 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] doki23 commented on pull request #4221: Nonstring partitioned cols
Posted by GitBox <gi...@apache.org>.
doki23 commented on PR #4221:
URL: https://github.com/apache/arrow-datafusion/pull/4221#issuecomment-1316164123
> Thank you @doki23 -- I think the functionality needs some test of a non-string partition column before we would consider merging it.
>
> Otherwise how would we know if we caused a regression (aka broke) this feature?
Thanks you. I did test them in delta lake but I forgot to add some unit tests for datafusion, I'll do 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org