alamb commented on code in PR #6135:
URL: https://github.com/apache/arrow-datafusion/pull/6135#discussion_r1180541040
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -528,6 +543,7 @@ pub fn make_binary_op_scalar_func(
pub fn to_substrait_rex(
Review Comment:
I think it might help to document what `col_ref_offset` means in this
context (like I think it is the schema offset, not the offset of columns within
the schema, for example) -- but maybe it is the total field offset 🤔
An example would help a lot
##########
datafusion/substrait/tests/roundtrip_logical_plan.rs:
##########
@@ -278,6 +278,18 @@ mod tests {
roundtrip("SELECT a,b,c,d,e FROM datafusion.public.data;").await
}
+ #[tokio::test]
+ async fn roundtrip_inner_join_table_reuse() -> Result<()> {
Review Comment:
Given there are offsets it might help to also test columns other than `a`
(which is at offset 0 in each schema)
Perhaps separate tests for the following cases would add better coverage:
* `SELECT d1.b, d2.c FROM data d1 JOIN data d2 ON d1.b = d2.b`
* `SELECT d1.b, d2.c FROM data d1 JOIN data d2 ON d1.b = d2.c`
##########
datafusion/substrait/tests/roundtrip_logical_plan.rs:
##########
@@ -278,6 +278,18 @@ mod tests {
roundtrip("SELECT a,b,c,d,e FROM datafusion.public.data;").await
}
+ #[tokio::test]
+ async fn roundtrip_inner_join_table_reuse() -> Result<()> {
Review Comment:
Also as I recall `USING` is also special so maybe a test for:
* `SELECT d1.b, d2.c FROM data d1 JOIN data d2 using (b)`
--
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]
