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
| 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.
There was a problem hiding this comment.
Incorporated both suggestions. Thank you @Blizzara
…ache#13862) * include FetchRel when producing LogicalPlan from Sort * add suggested test * address review feedback
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.