Support types other than String for partition columns on ListingTables #4221

doki23 · 2022-11-15T11:15:29Z

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 makes 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 tests including every *_with_partitions test.

Are there any user-facing changes?

Yes. Users can query partitioned columns by delta table definition.

mingmwang · 2022-11-15T16:07:44Z

I think not all the types of columns can be used as partition columns, should there a white list for supported types?

mingmwang · 2022-11-15T16:16:44Z

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?

alamb · 2022-11-15T19:42:43Z

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?

doki23 · 2022-11-16T01:45:53Z

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 schema of RecordBatch matches that of table definition @mingmwang .

doki23 · 2022-11-16T01:48:19Z

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?

Thank you. I did test them in delta lake but I forgot to add some unit tests for datafusion, I'll do it.

doki23 · 2022-11-16T05:31:51Z

It's because ListingTable treats type of partitioned columns as Utf8. I plan to change the function FileScanConfig.project() to provide a correct schema used by the physical plan created by ListingTable.

doki23 · 2022-11-21T09:39:38Z

@mingmwang @alamb Would you please help review my pr? I think it's ready now.

…nto nonstring-partitioned-cols

alamb

Thanks @doki23 -- this looks good to me. I had some style suggestions but nothing critical.

I'll plan to merge this PR tomorrow -- let me know if you want to delay so you can make any more changes

datafusion/core/src/datasource/listing_table_factory.rs

alamb · 2022-11-22T15:29:46Z

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),


alamb · 2022-11-22T15:34:11Z

datafusion/core/src/datasource/listing/table.rs

    /// ```
    pub fn with_table_partition_cols(
        mut self,
-        table_partition_cols: Vec<String>,
+        table_partition_cols: Vec<(String, DataType)>,


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

#[derive(Clone)] struct PartitionColumn { name: String, data_type: DataType }

alamb · 2022-11-22T15:34:23Z

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.",


doki23 · 2022-11-22T15:42:41Z

@alamb Thank you. I'll make more changes according to your suggestions tomorrow to make it better.

Co-authored-by: Andrew Lamb <[email protected]>

alamb

Looks good -- thank you @doki23

alamb · 2022-11-23T17:57:53Z

datafusion/core/src/datasource/listing/table.rs

+                    col.0.to_owned(),
+                    self.table_schema
+                        .field_with_name(&col.0)
+                        .unwrap()


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?

ursabot · 2022-11-23T18:02:36Z

Benchmark runs are scheduled for baseline = 502b7e3 and contender = 55bf8e9. 55bf8e9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
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

github-actions bot added the core Core DataFusion crate label Nov 15, 2022

alamb marked this pull request as draft November 15, 2022 19:42

doki23 added 10 commits November 17, 2022 13:07

partition columns config

60bec40

solve test compile problems

34f7b24

fix ut

b6aaf1f

add partition column types config for each file reader options

db310bc

get correct partition column types from partitioned files

f8c903d

remove DEFAULT_PARTITION_COLUMN_TYPE

deccd99

update pruned_partition_list

3a7fb40

change api

d0aa39c

add some tests

437c7a0

create partitioned external table with schema

44f1fb5

doki23 marked this pull request as ready for review November 21, 2022 09:38

doki23 added 2 commits November 21, 2022 18:09

Merge branch 'master' of https://github.com/apache/arrow-datafusion i…

6c7fea2

…nto nonstring-partitioned-cols

upd

b2e550b

alamb approved these changes Nov 22, 2022

View reviewed changes

doki23 and others added 3 commits November 23, 2022 08:49

Update datafusion/core/src/datasource/listing_table_factory.rs

31caa06

Co-authored-by: Andrew Lamb <[email protected]>

code clean

66e9b1b

code clean

a70ef66

doki23 requested a review from alamb November 23, 2022 01:19

alamb approved these changes Nov 23, 2022

View reviewed changes

alamb changed the title ~~Nonstring partitioned cols~~ Support types other than String for partition columns on ListingTables Nov 23, 2022

alamb merged commit 55bf8e9 into apache:master Nov 23, 2022

doki23 mentioned this pull request Nov 23, 2022

Throw error (not panic) if a listing table specifies an missing partition column #4350

Closed

doki23 deleted the nonstring-partitioned-cols branch March 24, 2023 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support types other than String for partition columns on ListingTables #4221

Support types other than String for partition columns on ListingTables #4221

doki23 commented Nov 15, 2022 •

edited

Loading

mingmwang commented Nov 15, 2022

mingmwang commented Nov 15, 2022

alamb commented Nov 15, 2022 •

edited

Loading

doki23 commented Nov 16, 2022 •

edited

Loading

doki23 commented Nov 16, 2022 •

edited

Loading

doki23 commented Nov 16, 2022 •

edited

Loading

doki23 commented Nov 21, 2022 •

edited

Loading

alamb left a comment

alamb Nov 22, 2022

alamb Nov 22, 2022

alamb Nov 22, 2022

doki23 commented Nov 22, 2022 •

edited

Loading

alamb left a comment

alamb Nov 23, 2022

ursabot commented Nov 23, 2022

Support types other than String for partition columns on ListingTables #4221

Support types other than String for partition columns on ListingTables #4221

Conversation

doki23 commented Nov 15, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mingmwang commented Nov 15, 2022

mingmwang commented Nov 15, 2022

alamb commented Nov 15, 2022 • edited Loading

doki23 commented Nov 16, 2022 • edited Loading

doki23 commented Nov 16, 2022 • edited Loading

doki23 commented Nov 16, 2022 • edited Loading

doki23 commented Nov 21, 2022 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 22, 2022

Choose a reason for hiding this comment

alamb Nov 22, 2022

Choose a reason for hiding this comment

alamb Nov 22, 2022

Choose a reason for hiding this comment

doki23 commented Nov 22, 2022 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 23, 2022

Choose a reason for hiding this comment

ursabot commented Nov 23, 2022

doki23 commented Nov 15, 2022 •

edited

Loading

alamb commented Nov 15, 2022 •

edited

Loading

doki23 commented Nov 16, 2022 •

edited

Loading

doki23 commented Nov 16, 2022 •

edited

Loading

doki23 commented Nov 16, 2022 •

edited

Loading

doki23 commented Nov 21, 2022 •

edited

Loading

doki23 commented Nov 22, 2022 •

edited

Loading