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/10 18:25:16 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #4170: Add ability to specify external sort information for ParquetExec
alamb opened a new pull request, #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170
TOOD:
- [ ] Write tests
# Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/4169
# Rationale for this change
Our parquet files are sorted but the new BasicEnforcement rule is trying to resort them as it doesn't know they are sorted. See more details in https://github.com/apache/arrow-datafusion/issues/4169
# What changes are included in this PR?
1. Add a way to specify an externally known sort order to ParquetExec
2. Tests that the basic enforcement rule follows them
# Are these changes tested?
Yes
# Are there any user-facing changes?
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1020503156
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
As I was working on this feature, it occurs to me that if the sort order is to be maintained, the ListingTable can't put multiple file into the same partition π€
The ListingTable does not attempt to read all files concurrently instead it will read files in sequence within a partition. This is an important property as it allows plans to run against 1000s of files and not try to open them all concurrently.
However, it means if we assign more than one file to a partitition the output sort order will not be preserved as illustrated in this diagrams
When only 1 file is assigned to each partition, each partition is correctly sorted on `(A, B, C)`
```text
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
β βββββββββββββββββ ββββββββββββββββ β ββββββββββββββββ β βββββββββββββββ β
β β 1.parquet β β β β 2.parquet β β β 3.parquet β β β 4.parquet β β
β β Sort: A, B, C β βSort: A, B, C β β βSort: A, B, C β β βSort: A, B, Cβ β
β βββββββββββββββββ β β ββββββββββββββββ β ββββββββββββββββ β βββββββββββββββ β
β β β β
β β β β β β
β β β β
β β β β β β
β β β β
β β β β β β
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
DataFusion DataFusion DataFusion DataFusion
β Partition 1 Partition 2 Partition 3 Partition 4 β
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
ParquetExec
```
When more than 1 file is assigned to each partition, each partition is NOT correctly sorted on `(A, B, C)`. Once the second file is scanned, the same values for A, B and C can be repeated in the same sorted stream
```text
β β β β β β β β β β β β β β β β β β β β β β β
β β β β β β β β β β β β β β β β β β β β β β
β βββββββββββββββββ ββββββββββββββββ β
β β 1.parquet β β β β 2.parquet β β
β β Sort: A, B, C β βSort: A, B, C β β
β βββββββββββββββββ β β ββββββββββββββββ β
β βββββββββββββββββ ββββββββββββββββ β
β β 3.parquet β β β β 4.parquet β β
β β Sort: A, B, C β βSort: A, B, C β β
β βββββββββββββββββ β β ββββββββββββββββ β
β β
β β β β
β β β β β β β β β β β β β β β β β β β β β
DataFusion DataFusion β
β Partition 1 Partition 1
β β β β β β β β β β β β β β β β β β β β β β β
ParquetExec
```
Does anyone have a preference between any of the possibilities:
1. Adding a check in DataFusion so that sorts will be ignored if there are more than one file in each partition?
2. Making more partitions if a sort order is specified
3. Making more partitions up to a point (maybe 2x the target partitions and if not ignore the sort order)?
This affects IOx, see https://github.com/influxdata/influxdb_iox/issues/6125
--
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 a diff in pull request #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
mingmwang commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1021046267
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
BTW, I don't see the logic to ignore the sort info if there are more than one files in each partition in this PR.
````
let (partitioned_file_lists, statistics) =
self.list_files_for_scan(ctx, filters, limit).await?;
// if no files need to be read, return an `EmptyExec`
if partitioned_file_lists.is_empty() {
let schema = self.schema();
let projected_schema = project_schema(&schema, projection.as_ref())?;
return Ok(Arc::new(EmptyExec::new(false, projected_schema)));
}
````
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1020503156
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
As I was working on this feature, it occurs to me that if the sort order is to be maintained, the ListingTable can't put multiple file into the same partition π€
The ListingTable does not attempt to read all files concurrently instead it will read files in sequence within a partition. This is an important property as it allows plans to run against 1000s of files and not try to open them all concurrently.
However, it means if we assign more than one file to a partitition the output sort order will not be preserved as illustrated in this diagrams
When only 1 file is assigned to each partition, each partition is correctly sorted on `(A, B, C)`
```text
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
β βββββββββββββββββ ββββββββββββββββ β ββββββββββββββββ β βββββββββββββββ β
β β 1.parquet β β β β 2.parquet β β β 3.parquet β β β 4.parquet β β
β β Sort: A, B, C β βSort: A, B, C β β βSort: A, B, C β β βSort: A, B, Cβ β
β βββββββββββββββββ β β ββββββββββββββββ β ββββββββββββββββ β βββββββββββββββ β
β β β β
β β β β β β
β β β β
β β β β β β
β β β β
β β β β β β
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
DataFusion DataFusion DataFusion DataFusion
β Partition 1 Partition 2 Partition 3 Partition 4 β
β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β β
ParquetExec
```
When more than 1 file is assigned to each partition, each partition is NOT correctly sorted on `(A, B, C)`. Once the second file is scanned, the same values for A, B and C can be repeated in the same sorted stream
```text
β β β β β β β β β β β β β β β β β β β β β β β
β β β β β β β β β β β β β β β β β β β β β β
β βββββββββββββββββ ββββββββββββββββ β
β β 1.parquet β β β β 2.parquet β β
β β Sort: A, B, C β βSort: A, B, C β β
β βββββββββββββββββ β β ββββββββββββββββ β
β βββββββββββββββββ ββββββββββββββββ β
β β 3.parquet β β β β 4.parquet β β
β β Sort: A, B, C β βSort: A, B, C β β
β βββββββββββββββββ β β ββββββββββββββββ β
β β
β β β β
β β β β β β β β β β β β β β β β β β β β β
DataFusion DataFusion β
β Partition 1 Partition 2
β β β β β β β β β β β β β β β β β β β β β β β
ParquetExec
```
Does anyone have a preference between any of the possibilities:
1. Adding a check in DataFusion so that sorts will be ignored if there are more than one file in each partition?
2. Making more partitions if a sort order is specified
3. Making more partitions up to a point (maybe 2x the target partitions and if not ignore the sort order)?
This affects IOx, see https://github.com/influxdata/influxdb_iox/issues/6125
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#issuecomment-1314115508
Benchmark runs are scheduled for baseline = d72b4f04cf12424a54b22bf376b87065d5fcb6aa and contender = d2814c960168b45c4a0f5d7bbb72d9f412cb08bd. d2814c960168b45c4a0f5d7bbb72d9f412cb08bd 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/f8d9d76ec1ea401182a3202a2a5ee746...a303de6e080d43c2a7e8542265c17e23/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/5abda29401254efcb893c242651e35c9...a570f145df2445aea8dcb4ef8bf7e762/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/5c40093461504c3b8735924574fed272...da2cc5ee3a9441fc97e61f364f1d04b3/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/f79a77c8551a45bdb329f9ba457cdb6f...3ae2b5109aa54e18847168cf82631868/)
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] mingmwang commented on a diff in pull request #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
mingmwang commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1021037377
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
I would prefer option (1) as well.
In future, we can implement a new SortPreservingParquetExec which can read multiple pre-sorted parquet files in a partition and leverage the efficient merge sort to keep the sort ordering. And during physical planning time, if the parent plan do needs the sort info, the physical planner can choose the SortPreservingParquetExec instead of normal ParquetExec.
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#issuecomment-1314100827
Thanks @mingmwang -- I am going to merge this one in as it is blocking me downstream and I will address any other follow on comments in other PRs. Thank you
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1020748593
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
π€ after thinking about this over the evening I think I like option 1 (ignore provided sort order if there is more than one file in each partition) the best
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1021478986
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
Added in [beab60e](https://github.com/apache/arrow-datafusion/pull/4170/commits/beab60e90cd81317f520cbc7e3e955f176acd3e6) and [d492b6f](https://github.com/apache/arrow-datafusion/pull/4170/commits/d492b6fc0fa212c694c4cc177bea956c324f34c4)
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#issuecomment-1312095294
@mingmwang if you have some time to review and comment on the design in this PR I would be appreciative
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1021478986
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
Added in [beab60e](https://github.com/apache/arrow-datafusion/pull/4170/commits/beab60e90cd81317f520cbc7e3e955f176acd3e6) and [d492b6f](https://github.com/apache/arrow-datafusion/pull/4170/commits/d492b6fc0fa212c694c4cc177bea956c324f34c4)
--
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 a diff in pull request #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
mingmwang commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1021040695
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -361,6 +377,46 @@ impl ListingTable {
pub fn options(&self) -> &ListingOptions {
&self.options
}
+
+ /// If file_sort_order is specified, creates the appropriate physical expressions
+ fn try_create_output_ordering(&self) -> Result<Option<Vec<PhysicalSortExpr>>> {
+ let file_sort_order =
+ if let Some(file_sort_order) = self.options.file_sort_order.as_ref() {
+ file_sort_order
+ } else {
+ return Ok(None);
+ };
+
+ // convert each expr to a physical sort expr
+ let sort_exprs = file_sort_order
+ .iter()
+ .map(|expr| {
+ if let Expr::Sort { expr, asc, nulls_first } = expr {
+ if let Expr::Column(col) = expr.as_ref() {
+ let expr = physical_plan::expressions::col(&col.name, self.table_schema.as_ref())?;
+ Ok(PhysicalSortExpr {
+ expr,
+ options: SortOptions {
+ descending: !asc,
+ nulls_first: *nulls_first,
+ },
+ })
+ }
+ else {
+ Err(DataFusionError::Plan(
+ format!("Only support single column references in output_ordering, got {:?}", expr)
+ ))
+ }
+ } else {
+ Err(DataFusionError::Plan(
+ format!("Expected Expr::Sort in output_ordering, but got {:?}", expr)
+ ))
+ }
+ })
Review Comment:
Should we return Error to make the plan failed or just log some error info and return Ok(None) ?
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
mingmwang commented on PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#issuecomment-1313755330
LGTM.
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1021438829
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
You are correct -- I will add the logic to ignore sort order to this PR
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -361,6 +377,46 @@ impl ListingTable {
pub fn options(&self) -> &ListingOptions {
&self.options
}
+
+ /// If file_sort_order is specified, creates the appropriate physical expressions
+ fn try_create_output_ordering(&self) -> Result<Option<Vec<PhysicalSortExpr>>> {
+ let file_sort_order =
+ if let Some(file_sort_order) = self.options.file_sort_order.as_ref() {
+ file_sort_order
+ } else {
+ return Ok(None);
+ };
+
+ // convert each expr to a physical sort expr
+ let sort_exprs = file_sort_order
+ .iter()
+ .map(|expr| {
+ if let Expr::Sort { expr, asc, nulls_first } = expr {
+ if let Expr::Column(col) = expr.as_ref() {
+ let expr = physical_plan::expressions::col(&col.name, self.table_schema.as_ref())?;
+ Ok(PhysicalSortExpr {
+ expr,
+ options: SortOptions {
+ descending: !asc,
+ nulls_first: *nulls_first,
+ },
+ })
+ }
+ else {
+ Err(DataFusionError::Plan(
+ format!("Only support single column references in output_ordering, got {:?}", expr)
+ ))
+ }
+ } else {
+ Err(DataFusionError::Plan(
+ format!("Expected Expr::Sort in output_ordering, but got {:?}", expr)
+ ))
+ }
+ })
Review Comment:
I personally prefer returning an error here as it would signify some bug in the specification of the sort expression that should be fixed by the caller -- but I can also see the rationale for being more forgiving.
If/When we start reading sort order from the parquet files themselves then I think `warn!` and continuing without error would definitely be a better behavior
We could also make some setting that controlled the behavior (error or ignore). Would that be 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] mingmwang commented on pull request #4170: Add ability to specify external sort information for ParquetExec
Posted by GitBox <gi...@apache.org>.
mingmwang commented on PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#issuecomment-1311121354
π
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#discussion_r1021489372
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -220,6 +225,16 @@ pub struct ListingOptions {
/// Group files to avoid that the number of partitions exceeds
/// this limit
pub target_partitions: usize,
+ /// Optional pre-known sort order. Must be `SortExpr`s.
+ ///
+ /// DataFusion may take advantage of this ordering to omit sorts
+ /// or use more efficient algorithms. Currently sortedness must be
+ /// provided if it is known by some external mechanism, but may in
+ /// the future be automatically determined, for example using
+ /// parquet metadata.
+ ///
+ /// See <https://github.com/apache/arrow-datafusion/issues/4177>
+ pub file_sort_order: Option<Vec<Expr>>,
Review Comment:
Added
--
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 #4170: Add ability to specify external sort information for ParquetExec
Posted by GitBox <gi...@apache.org>.
mingmwang commented on PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170#issuecomment-1311121816
Yes, if the parquet files are already sorted, we should expose the info to the optimizer.
--
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 #4170: Add ability to specify external sort information for ListingTables
Posted by GitBox <gi...@apache.org>.
alamb merged PR #4170:
URL: https://github.com/apache/arrow-datafusion/pull/4170
--
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