-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Support Copy To Partitioned Files #9240
Changes from all commits
f1dcbf9
7f017d6
109471b
6c68f4b
e9d6325
aed4d08
8b6b6b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ use arrow_array::cast::AsArray; | |
use arrow_array::{downcast_dictionary_array, RecordBatch, StringArray, StructArray}; | ||
use arrow_schema::{DataType, Schema}; | ||
use datafusion_common::cast::as_string_array; | ||
use datafusion_common::DataFusionError; | ||
use datafusion_common::{exec_datafusion_err, DataFusionError}; | ||
|
||
use datafusion_execution::TaskContext; | ||
|
||
|
@@ -319,14 +319,20 @@ fn compute_partition_keys_by_row<'a>( | |
) -> Result<Vec<Vec<&'a str>>> { | ||
let mut all_partition_values = vec![]; | ||
|
||
for (col, dtype) in partition_by.iter() { | ||
// For the purposes of writing partitioned data, we can rely on schema inference | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
// to determine the type of the partition cols in order to provide a more ergonomic | ||
// UI which does not require specifying DataTypes manually. So, we ignore the | ||
// DataType within the partition_by array and infer the correct type from the | ||
// batch schema instead. | ||
let schema = rb.schema(); | ||
for (col, _) in partition_by.iter() { | ||
let mut partition_values = vec![]; | ||
let col_array = | ||
rb.column_by_name(col) | ||
.ok_or(DataFusionError::Execution(format!( | ||
"PartitionBy Column {} does not exist in source data!", | ||
col | ||
)))?; | ||
|
||
let dtype = schema.field_with_name(col)?.data_type(); | ||
let col_array = rb.column_by_name(col).ok_or(exec_datafusion_err!( | ||
"PartitionBy Column {} does not exist in source data! Got schema {schema}.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it can be shortened with |
||
col | ||
))?; | ||
|
||
match dtype { | ||
DataType::Utf8 => { | ||
|
@@ -339,12 +345,12 @@ fn compute_partition_keys_by_row<'a>( | |
downcast_dictionary_array!( | ||
col_array => { | ||
let array = col_array.downcast_dict::<StringArray>() | ||
.ok_or(DataFusionError::Execution(format!("it is not yet supported to write to hive partitions with datatype {}", | ||
dtype)))?; | ||
.ok_or(exec_datafusion_err!("it is not yet supported to write to hive partitions with datatype {}", | ||
dtype))?; | ||
|
||
for val in array.values() { | ||
partition_values.push( | ||
val.ok_or(DataFusionError::Execution(format!("Cannot partition by null value for column {}", col)))? | ||
val.ok_or(exec_datafusion_err!("Cannot partition by null value for column {}", col))? | ||
); | ||
} | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -568,6 +568,7 @@ impl DefaultPhysicalPlanner { | |
output_url, | ||
file_format, | ||
copy_options, | ||
partition_by, | ||
}) => { | ||
let input_exec = self.create_initial_plan(input, session_state).await?; | ||
let parsed_url = ListingTableUrl::parse(output_url)?; | ||
|
@@ -585,13 +586,20 @@ impl DefaultPhysicalPlanner { | |
CopyOptions::WriterOptions(writer_options) => *writer_options.clone() | ||
}; | ||
|
||
// Note: the DataType passed here is ignored for the purposes of writing and inferred instead | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The read path needs an explicit DataType defined for the partition cols so it knows what to cast to, but I realized that the write path can just infer the correct DataType from the RecordBatch schema. This allows COPY to only specify partition columns by name and not have to worry about specifying the correct data type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree this is a better UX -- thank you |
||
// from the schema of the RecordBatch being written. This allows COPY statements to specify only | ||
// the column name rather than column name + explicit data type. | ||
let table_partition_cols = partition_by.iter() | ||
.map(|s| (s.to_string(), arrow_schema::DataType::Null)) | ||
.collect::<Vec<_>>(); | ||
|
||
// Set file sink related options | ||
let config = FileSinkConfig { | ||
object_store_url, | ||
table_paths: vec![parsed_url], | ||
file_groups: vec![], | ||
output_schema: Arc::new(schema), | ||
table_partition_cols: vec![], | ||
table_partition_cols, | ||
overwrite: false, | ||
file_type_writer_options | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -918,6 +918,7 @@ impl AsLogicalPlan for LogicalPlanNode { | |
input: Arc::new(input), | ||
output_url: copy.output_url.clone(), | ||
file_format: FileType::from_str(©.file_type)?, | ||
partition_by: vec![], | ||
copy_options, | ||
}, | ||
)) | ||
|
@@ -1641,6 +1642,7 @@ impl AsLogicalPlan for LogicalPlanNode { | |
output_url, | ||
file_format, | ||
copy_options, | ||
partition_by: _, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that I did not add support for partition_by in proto. We should add a follow up ticket for this. I don't believe this PR will break downstream systems like Ballista's handling of COPY, but it will silently ignore partition_by options until prost is updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filed #9248 |
||
}) => { | ||
let input = protobuf::LogicalPlanNode::try_from_logical_plan( | ||
input, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,90 @@ COPY source_table TO 'test_files/scratch/copy/table/' (format parquet, compressi | |
---- | ||
2 | ||
|
||
# Copy to directory as partitioned files | ||
query IT | ||
COPY source_table TO 'test_files/scratch/copy/partitioned_table1/' (format parquet, compression 'zstd(10)', partition_by 'col2'); | ||
---- | ||
2 | ||
|
||
# validate multiple partitioned parquet file output | ||
statement ok | ||
CREATE EXTERNAL TABLE validate_partitioned_parquet STORED AS PARQUET | ||
LOCATION 'test_files/scratch/copy/partitioned_table1/' PARTITIONED BY (col2); | ||
|
||
query I? | ||
select * from validate_partitioned_parquet order by col1, col2; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add a test for reading out of one of the partitions Something like select * from 'test_files/scratch/copy/partitioned_table1/col2=Foo' To demonstrate that the output was actually partitioned ? I think this test would pass even if the partition columns were ignored There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added additional tests to copy.slt to verify this |
||
---- | ||
1 Foo | ||
2 Bar | ||
|
||
# validate partition paths were actually generated | ||
statement ok | ||
CREATE EXTERNAL TABLE validate_partitioned_parquet_bar STORED AS PARQUET | ||
LOCATION 'test_files/scratch/copy/partitioned_table1/col2=Bar'; | ||
|
||
query I | ||
select * from validate_partitioned_parquet_bar order by col1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
---- | ||
2 | ||
|
||
# Copy to directory as partitioned files | ||
query ITT | ||
COPY (values (1, 'a', 'x'), (2, 'b', 'y'), (3, 'c', 'z')) TO 'test_files/scratch/copy/partitioned_table2/' | ||
(format parquet, compression 'zstd(10)', partition_by 'column2, column3'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anonymous columns get the name "columnX" based on their order in the VALUES clause. It would be nice to document this somewhere, though I did make sure it is relatively easy to discover this based on the error message if you get a column name wrong. |
||
---- | ||
3 | ||
|
||
# validate multiple partitioned parquet file output | ||
statement ok | ||
CREATE EXTERNAL TABLE validate_partitioned_parquet2 STORED AS PARQUET | ||
LOCATION 'test_files/scratch/copy/partitioned_table2/' PARTITIONED BY (column2, column3); | ||
|
||
query I?? | ||
select * from validate_partitioned_parquet2 order by column1,column2,column3; | ||
---- | ||
1 a x | ||
2 b y | ||
3 c z | ||
|
||
statement ok | ||
CREATE EXTERNAL TABLE validate_partitioned_parquet_a_x STORED AS PARQUET | ||
LOCATION 'test_files/scratch/copy/partitioned_table2/column2=a/column3=x'; | ||
|
||
query I | ||
select * from validate_partitioned_parquet_a_x order by column1; | ||
---- | ||
1 | ||
|
||
statement ok | ||
create table test ("'test'" varchar, "'test2'" varchar, "'test3'" varchar); | ||
|
||
query TTT | ||
insert into test VALUES ('a', 'x', 'aa'), ('b','y', 'bb'), ('c', 'z', 'cc') | ||
---- | ||
3 | ||
|
||
query T | ||
select "'test'" from test | ||
---- | ||
a | ||
b | ||
c | ||
|
||
# Note to place a single ' inside of a literal string escape by putting two '' | ||
query TTT | ||
copy test to 'test_files/scratch/copy/escape_quote' (format csv, partition_by '''test2'',''test3''') | ||
---- | ||
3 | ||
|
||
statement ok | ||
CREATE EXTERNAL TABLE validate_partitioned_escape_quote STORED AS CSV | ||
LOCATION 'test_files/scratch/copy/escape_quote/' PARTITIONED BY ("'test2'", "'test3'"); | ||
|
||
# This triggers a panic (index out of bounds) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying to query on this table partitioned by a single quote containing column name panics with index out of bounds error. Manually inspecting the CSV suggests the previous COPY statement worked. As mentioned in other thread, I'm not sure if it makes sense to support ' in a partition path name. It will certainly get ugly if we try. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally I think we wouldn't panic (perhaps we can generate a not supported error instead) Given this PR doesn't seem to make the situation worse (or better) I don't think we need to fix it now. Instead I think we should file a ticket to address it as a follow on. I will do so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
#query | ||
#select * from validate_partitioned_escape_quote; | ||
|
||
query TT | ||
EXPLAIN COPY source_table TO 'test_files/scratch/copy/table/' (format parquet, compression 'zstd(10)'); | ||
---- | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a test for this new feature
DataFrame::write_parquet
I took a look around and I didn't see any good existing tests sadly. This is what I found.
https://github.com/apache/arrow-datafusion/blob/4d389c2590370d85bfe3af77f5243d5b40f5a222/datafusion/core/src/datasource/physical_plan/parquet/mod.rs#L2070
I'll make a short PR to move those tests into the dataframe tests to make it more discoverable