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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
