crepererum commented on code in PR #17439:
URL: https://github.com/apache/datafusion/pull/17439#discussion_r2333254573
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -383,7 +383,7 @@ impl FileScanConfigBuilder {
/// Set the output ordering of the files
pub fn with_output_ordering(mut self, output_ordering: Vec<LexOrdering>)
-> Self {
Review Comment:
Could we eventually fix this API as well so that the API setter is the same
as the internal state? If you wanna make the transition for existing users
smoother, deprecate this method here and add a new one a la:
```rust
pub fn with_output_ordering_opt(mut self, output_ordering: Vec<LexOrdering>)
-> Self { ... }
```
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -1044,7 +1054,9 @@ impl DisplayAs for FileScanConfig {
write!(f, ", limit={limit}")?;
}
- display_orderings(f, &orderings)?;
+ let flattened_orderings: Vec<LexOrdering> =
Review Comment:
same as for the other display code.
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -528,7 +528,12 @@ impl DataSource for FileScanConfig {
match t {
DisplayFormatType::Default | DisplayFormatType::Verbose => {
let schema = self.projected_schema();
- let orderings = get_projected_output_ordering(self, &schema);
+ // Remove None from projected output ordering
Review Comment:
I think we should display the `None` cases too, otherwise this is going to
be pretty confusing. Imagine a projection that leads to `Some, None, Some` and
you now gonna display 2 orderings (for 3 partitions). From that you cannot tell
if it was `None, Some, Some`, `Some, None, Some`, or `Some, Some, None`.
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -785,19 +790,24 @@ impl FileScanConfig {
/// Project the schema, constraints, and the statistics on the given
column indices
pub fn project(&self) -> (SchemaRef, Constraints, Statistics,
Vec<LexOrdering>) {
if self.projection.is_none() && self.table_partition_cols.is_empty() {
+ let output_ordering: Vec<LexOrdering> =
Review Comment:
I'm not convinced that this is correct, with the same argument as for the
display code. I think the return type here also needs to be `Vec<Option<...>>`.
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
