Re: [PR] fix(substrait): remove optimize calls from substrait consumer [datafusion]

["Blizzara (via GitHub)" <gi...@apache.org>]

Blizzara commented on code in PR #12800:
URL: https://github.com/apache/datafusion/pull/12800#discussion_r1792099612


##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -783,7 +783,6 @@ pub async fn from_substrait_rel(
 
                 let t = ctx.table(table_reference.clone()).await?;
                 let t = ensure_schema_compatability(t, substrait_schema)?;
-                let t = t.into_optimized_plan()?;
                 extract_projection(t, &read.projection)

Review Comment:
   > @Blizzara I'm thinking of switching gears a bit and curious what you think. I think the easiest solution here will be to first "apply" substrait projections to substrait base_schema and only then use `ensure_schema_compatability` with the resulting trimmed down schema as an input.
   
   I think that'd be fine by me!
   
   > One thing I'm unsure about is what to do when data types between table schema and a substrait base_schema do not match for a field which is projected away by substrait anyway. For example
   > 
   > * table has schema (a int, b str)
   > * substrait base_schema is (a int, b int) # note that b has an incorrect type
   > * substrait projections field is [0]
   > 
   > If I trim down base_schema first and only then check compatibility with the actual table, this won't throw an error, it will be as if column b was never mentioned in the plan at all. wdyt? Does that sound like the "right" behavior?
   
   Also fine by me, given as long as the plan can safely execute (all columns used in any way exist and are of right type) then I don't see a need to fail that. But one could as well argue the whole included schema should be validated for consistency reasons, so dunno if @westonpace @vbarua have stricter opinions here?
   
   > (On a related note, I think the same needs to applied to LocalFiles ReadType as well, which for some reason right now outright ignores base_schema. I think if we allow NamedTable base_schema to be different from the actual table schema, makes sense to do the same for LocalFiles).
   
   Yeah, it wasn't done yet just to keep the PR smaller, but agreed the behavior should be same across all source types.



-- 
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