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


##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -42,7 +42,7 @@ use datafusion::logical_expr::expr::{
 };
 use datafusion::logical_expr::{expr, Between, JoinConstraint, LogicalPlan, 
Operator};
 use datafusion::prelude::Expr;
-use prost_types::Any as ProtoAny;
+use pbjson_types::Any as ProtoAny;

Review Comment:
   So this confuses me a bit as well - but the way I understand it, adding the 
"serde" feature to "substrait-rs" seems to change the types it expects from the 
original `prost` types into the `pbjson` types 
(https://github.com/substrait-io/substrait-rs/blob/d11b22c743df9b02b7e5f4c079091ed133982431/build.rs#L262),
 probably to make them all serde-serializable 
(https://docs.rs/pbjson-types/latest/pbjson_types/#). 
   
   Given we use the `Any` proto type in non-test code, I think this does need 
to change, even though we don't necessarily need the serializability here.. 
This is also why we need the pbjson-types dep as normal, not dev, dep.
   
   I'd also prefer to not change this, but I couldn't find another way if we 
want to have the JSON proto plans :/



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

Reply via email to