You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "crepererum (via GitHub)" <gi...@apache.org> on 2025/07/15 11:04:02 UTC
[PR] fix: support nullable columns in pre-sorted data sources [datafusion]
crepererum opened a new pull request, #16783:
URL: https://github.com/apache/datafusion/pull/16783
## Which issue does this PR close?
\-
## Rationale for this change
There are valid cases where files are pre-sorted on nullable columns. Arrow and DataFusion have proper semantic for that.
## What changes are included in this PR?
Removes an exception / special-handling for nullable columns. The Arrow row encoder that is used in the respective code fully supports nullable columns (or at least it will cleanly fail if it doesn't), INCLUDING nulls-first&last.
## Are these changes tested?
Regression test (added in 1st commit, fixed in 2nd commit)
## Are there any user-facing changes?
Some queries are now faster.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support nullable columns in pre-sorted data sources [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16783:
URL: https://github.com/apache/datafusion/pull/16783#discussion_r2208603160
##########
datafusion/datasource/src/statistics.rs:
##########
@@ -230,14 +230,7 @@ impl MinMaxStatistics {
.zip(sort_columns.iter().copied())
.map(|(sort_expr, column)| {
let schema = values.schema();
-
Review Comment:
This appears to be the only actual code change: remove these lines
`git praise` says they came in via #9593 from @suremarc . Do you remember why this condition was added @suremarc ?
https://github.com/apache/datafusion/blame/a614716e7d97ff1d3374aef31b9a66fd10141423/datafusion/datasource/src/statistics.rs#L238
##########
datafusion/sqllogictest/test_files/parquet.slt:
##########
@@ -130,8 +130,7 @@ STORED AS PARQUET;
----
3
-# Check output plan again, expect no "output_ordering" clause in the physical_plan -> ParquetExec,
-# due to there being more files than partitions:
+# Check output plan again
Review Comment:
I agree -- I reviewed the test and the table definition explicitly says
```sql
WITH ORDER (string_col ASC NULLS LAST, int_col ASC NULLS LAST)
```
So I would expect this plan not to have additional sorting
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support nullable columns in pre-sorted data sources [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16783:
URL: https://github.com/apache/datafusion/pull/16783#issuecomment-3085555738
Thanks @crepererum
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support nullable columns in pre-sorted data sources [datafusion]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #16783:
URL: https://github.com/apache/datafusion/pull/16783#discussion_r2207466414
##########
datafusion/sqllogictest/test_files/parquet.slt:
##########
@@ -130,8 +130,7 @@ STORED AS PARQUET;
----
3
-# Check output plan again, expect no "output_ordering" clause in the physical_plan -> ParquetExec,
-# due to there being more files than partitions:
+# Check output plan again
Review Comment:
I think the actual reason why the `output_ordering` was missing here wasn't the number of files, but because DF had trouble with a column that wasn't marked as `NOT NULL` (i.e. is nullable).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: support nullable columns in pre-sorted data sources [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #16783:
URL: https://github.com/apache/datafusion/pull/16783
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org