alamb commented on code in PR #7743:
URL: https://github.com/apache/arrow-datafusion/pull/7743#discussion_r1349298428
##########
datafusion/physical-plan/src/insert.rs:
##########
@@ -73,6 +73,8 @@ pub struct FileSinkExec {
sink_schema: SchemaRef,
/// Schema describing the structure of the output data.
count_schema: SchemaRef,
+ /// Optional required sort order for output data.
+ sort_order: Option<Vec<Option<Vec<PhysicalSortRequirement>>>>,
Review Comment:
Since FileSink can have only a single input, I think it only needs a single
sort order per
[`required_input_order`](https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#method.required_input_ordering)
In other words, I think this could be simplified to
```suggestion
sort_order: Option<Vec<PhysicalSortRequirement>>>,
```
And then adjust required_input_order appropriately
##########
datafusion/sqllogictest/test_files/insert_to_external.slt:
##########
@@ -45,6 +45,35 @@ LOCATION '../../testing/data/csv/aggregate_test_100.csv'
statement ok
set datafusion.execution.target_partitions = 8;
+statement ok
+CREATE EXTERNAL TABLE
+ordered_insert_test(a bigint, b bigint)
+STORED AS csv
+LOCATION 'test_files/scratch/insert_to_external/insert_to_ordered/'
+WITH ORDER (a ASC, B DESC)
+OPTIONS(
+create_local_path 'true',
+insert_mode 'append_new_files',
+);
+
+query II
+INSERT INTO ordered_insert_test values (5, 1), (4, 2), (7,7), (7,8), (7,9),
(7,10), (3, 3), (2, 4), (1, 5);
+----
+9
Review Comment:
Can you please also add an `EXPLAIN INSERT INTO ordered_insert_test values
(5, 1), (4, 2)` to verify that the plan has a `SortExec in it?
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -604,7 +604,7 @@ impl DefaultPhysicalPlanner {
FileType::ARROW => Arc::new(ArrowFormat {}),
};
- sink_format.create_writer_physical_plan(input_exec,
session_state, config).await
+ sink_format.create_writer_physical_plan(input_exec,
session_state, config, None).await
Review Comment:
👍
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -604,7 +604,7 @@ impl DefaultPhysicalPlanner {
FileType::ARROW => Arc::new(ArrowFormat {}),
};
- sink_format.create_writer_physical_plan(input_exec,
session_state, config).await
+ sink_format.create_writer_physical_plan(input_exec,
session_state, config, None).await
Review Comment:
👍
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
