You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "robtandy (via GitHub)" <gi...@apache.org> on 2024/12/20 18:48:01 UTC
[PR] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
robtandy opened a new pull request, #13862:
URL: https://github.com/apache/datafusion/pull/13862
## 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_plan` will include a `FetchRel` when serializing a `Sort` that contains a `fetch`. It had previously missed this.
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "robtandy (via GitHub)" <gi...@apache.org>.
robtandy commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894417992
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -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
Review Comment:
added
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "robtandy (via GitHub)" <gi...@apache.org>.
robtandy commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894640008
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -368,14 +368,45 @@ pub fn to_substrait_rel(
.iter()
.map(|e| substrait_sort_field(state, e, sort.input.schema(), extensions))
.collect::<Result<Vec<_>>>()?;
- Ok(Box::new(Rel {
+
+ let sort_rel = Box::new(Rel {
rel_type: Some(RelType::Sort(Box::new(SortRel {
common: None,
input: Some(input),
sorts: sort_fields,
advanced_extension: None,
}))),
- }))
+ });
+
+ match sort.fetch {
+ Some(_) => {
+ let empty_schema = Arc::new(DFSchema::empty());
+ let count_mode = sort
+ .fetch
+ .map(|amount| {
Review Comment:
Incorporated both suggestions. Thank you @Blizzara
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "robtandy (via GitHub)" <gi...@apache.org>.
robtandy commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894418929
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -368,14 +368,45 @@ pub fn to_substrait_rel(
.iter()
.map(|e| substrait_sort_field(state, e, sort.input.schema(), extensions))
.collect::<Result<Vec<_>>>()?;
- Ok(Box::new(Rel {
+
+ let sort_rel = Box::new(Rel {
rel_type: Some(RelType::Sort(Box::new(SortRel {
common: None,
input: Some(input),
sorts: sort_fields,
advanced_extension: None,
}))),
- }))
+ });
+
+ match sort.fetch {
+ Some(_) => {
+ let empty_schema = Arc::new(DFSchema::empty());
+ let count_mode = sort
+ .fetch
+ .map(|amount| {
Review Comment:
ty for the suggestions! Heading to dinner now and I can address these tomorrow morning eastern time.
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "robtandy (via GitHub)" <gi...@apache.org>.
robtandy commented on PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#issuecomment-2557560399
Per contribution guide
```For new contributors a committer must first trigger the CI tasks. Please mention the members from committers list in the PR to help trigger the CI```
Mentioning @alamb :)
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "robtandy (via GitHub)" <gi...@apache.org>.
robtandy commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894410849
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -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
Review Comment:
Good idea. I'll add.
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -368,14 +368,45 @@ pub fn to_substrait_rel(
.iter()
.map(|e| substrait_sort_field(state, e, sort.input.schema(), extensions))
.collect::<Result<Vec<_>>>()?;
- Ok(Box::new(Rel {
+
+ let sort_rel = Box::new(Rel {
rel_type: Some(RelType::Sort(Box::new(SortRel {
common: None,
input: Some(input),
sorts: sort_fields,
advanced_extension: None,
}))),
- }))
+ });
+
+ match sort.fetch {
Review Comment:
Making sure I follow:
You mean on line 364 destructuring the plan like
```rust
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
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894409947
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -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
Review Comment:
It might also be worth a test for `OFFSET`
Like
```sql
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.
```
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -368,14 +368,45 @@ pub fn to_substrait_rel(
.iter()
.map(|e| substrait_sort_field(state, e, sort.input.schema(), extensions))
.collect::<Result<Vec<_>>>()?;
- Ok(Box::new(Rel {
+
+ let sort_rel = Box::new(Rel {
rel_type: Some(RelType::Sort(Box::new(SortRel {
common: None,
input: Some(input),
sorts: sort_fields,
advanced_extension: None,
}))),
- }))
+ });
+
+ match sort.fetch {
Review Comment:
👍
Another pattern I have seen to avoid missing fields like this is
```rust
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
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "Blizzara (via GitHub)" <gi...@apache.org>.
Blizzara commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894416585
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -368,14 +368,45 @@ pub fn to_substrait_rel(
.iter()
.map(|e| substrait_sort_field(state, e, sort.input.schema(), extensions))
.collect::<Result<Vec<_>>>()?;
- Ok(Box::new(Rel {
+
+ let sort_rel = Box::new(Rel {
rel_type: Some(RelType::Sort(Box::new(SortRel {
common: None,
input: Some(input),
sorts: sort_fields,
advanced_extension: None,
}))),
- }))
+ });
+
+ match sort.fetch {
+ Some(_) => {
+ let empty_schema = Arc::new(DFSchema::empty());
+ let count_mode = sort
+ .fetch
+ .map(|amount| {
Review Comment:
also I think rather than using to_substrait_rex, you could create the substrait expr directly, that might be nicer here
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "Blizzara (via GitHub)" <gi...@apache.org>.
Blizzara commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894415637
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -368,14 +368,45 @@ pub fn to_substrait_rel(
.iter()
.map(|e| substrait_sort_field(state, e, sort.input.schema(), extensions))
.collect::<Result<Vec<_>>>()?;
- Ok(Box::new(Rel {
+
+ let sort_rel = Box::new(Rel {
rel_type: Some(RelType::Sort(Box::new(SortRel {
common: None,
input: Some(input),
sorts: sort_fields,
advanced_extension: None,
}))),
- }))
+ });
+
+ match sort.fetch {
+ Some(_) => {
+ let empty_schema = Arc::new(DFSchema::empty());
+ let count_mode = sort
+ .fetch
+ .map(|amount| {
Review 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), ...)
```
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13862:
URL: https://github.com/apache/datafusion/pull/13862
--
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] Minor: fix: Include FetchRel when producing LogicalPlan from Sort [datafusion]
Posted by "robtandy (via GitHub)" <gi...@apache.org>.
robtandy commented on code in PR #13862:
URL: https://github.com/apache/datafusion/pull/13862#discussion_r1894639389
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -368,14 +368,45 @@ pub fn to_substrait_rel(
.iter()
.map(|e| substrait_sort_field(state, e, sort.input.schema(), extensions))
.collect::<Result<Vec<_>>>()?;
- Ok(Box::new(Rel {
+
+ let sort_rel = Box::new(Rel {
rel_type: Some(RelType::Sort(Box::new(SortRel {
common: None,
input: Some(input),
sorts: sort_fields,
advanced_extension: None,
}))),
- }))
+ });
+
+ match sort.fetch {
Review Comment:
Done
--
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