blaginin commented on code in PR #17900:
URL: https://github.com/apache/datafusion/pull/17900#discussion_r2404156038
##########
datafusion/core/tests/physical_optimizer/enforce_sorting.rs:
##########
@@ -505,29 +571,35 @@ async fn test_union_inputs_different_sorted6() ->
Result<()> {
// At the same time, this ordering requirement is unnecessarily fine.
// The final plan should be valid AND the ordering of the third child
// shouldn't be finer than necessary.
- let expected_input = [
- "SortPreservingMergeExec: [nullable_col@0 ASC]",
- " UnionExec",
- " SortExec: expr=[nullable_col@0 ASC],
preserve_partitioning=[false]",
- " DataSourceExec: file_groups={1 group: [[x]]},
projection=[nullable_col, non_nullable_col], file_type=parquet",
- " DataSourceExec: file_groups={1 group: [[x]]},
projection=[nullable_col, non_nullable_col], output_ordering=[nullable_col@0
ASC], file_type=parquet",
- " SortPreservingMergeExec: [nullable_col@0 ASC, non_nullable_col@1
ASC]",
- " RepartitionExec: partitioning=RoundRobinBatch(10),
input_partitions=1",
- " DataSourceExec: file_groups={1 group: [[x]]},
projection=[nullable_col, non_nullable_col], file_type=parquet",
- ];
+ let test = EnforceSortingTest::new(physical_plan)
+ .with_repartition_sorts(true)
+ .with_expected_description(
+ "// Should adjust the requirement in the third input of the union
so\n\
+ // that it is not unnecessarily fine.",
Review Comment:
Feel free to ignore, but I think it's not the best idea to mix comments and
snapshots - it'll now be repeated twice in the code, making it messier, and it
won't be clear for newcomers which one to update
I personally don’t see an issue with just adding the comment after the
assert. I did that separately in:
https://github.com/blaginin/datafusion/commit/ecdf6c3ee15a05cfe654d64b00d3e49dbeb59e12
in case you’ll like it🙂
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
