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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
