Minor: fix: Include FetchRel when producing LogicalPlan from Sort #13862
Conversation
|
Per contribution guide Mentioning @alamb :) |
| })) | ||
| }); | ||
|
|
||
| match sort.fetch { |
There was a problem hiding this comment.
👍
Another pattern I have seen to avoid missing fields like this is
let Sort { input, offset } = sort ;
...That way the compiler will tell you when you have not handled some field and you have to explicitly list it out
There was a problem hiding this comment.
Making sure I follow:
You mean on line 364 destructuring the plan like
LogicalPlan::Sort(Sort {expr, input, fetch}) => {...I didn't see that pattern followed in that file for the other LogicalPlan inners. I think its a good idea though
| @@ -200,6 +200,11 @@ async fn select_with_filter() -> Result<()> { | |||
| roundtrip("SELECT * FROM data WHERE a > 1").await | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn select_with_filter_sort_limit() -> Result<()> { | |||
| roundtrip("SELECT * FROM data WHERE a > 1 ORDER BY b ASC LIMIT 2").await | |||
There was a problem hiding this comment.
It might also be worth a test for OFFSET
Like
roundtrip("SELECT * FROM data WHERE a > 1 ORDER BY b ASC LIMIT 2").await> select * from values (1), (2), (3) ORDER BY column1 LIMIT 1 offset 2;
+---------+
| column1 |
+---------+
| 3 |
+---------+
1 row(s) fetched.
Elapsed 0.012 seconds.
| let count_mode = sort | ||
| .fetch | ||
| .map(|amount| { |
There was a problem hiding this comment.
given you have the match sort.fetch { Some(_) => above, you could store that "some" and then use it here
match sort.fetch {
Some(amount) => {
...
let count_mode = to_substrait_rex(.., (amount as i64), ...)
There was a problem hiding this comment.
also I think rather than using to_substrait_rex, you could create the substrait expr directly, that might be nicer here
There was a problem hiding this comment.
ty for the suggestions! Heading to dinner now and I can address these tomorrow morning eastern time.
Which issue does this PR close?
Closes #13860
Rationale for this change
Explained in #13860
What changes are included in this PR?
Are these changes tested?
Included a test that fails before this change
Are there any user-facing changes?
to_substrait_planwill include aFetchRelwhen serializing aSortthat contains afetch. It had previously missed this.