Re: [PR] Implement TPCH substrait integration test, support tpch_4 and tpch_5 [datafusion]

[via GitHub]

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


##########
datafusion/substrait/tests/cases/consumer_integration.rs:
##########
@@ -90,6 +90,40 @@ mod tests {
         Ok(ctx)
     }
 
+    async fn create_context_tpch4() -> Result<SessionContext> {
+        let ctx = SessionContext::new();
+
+        let registrations = vec![

Review Comment:
   doesn't need to be part of this PR, but how about having a general 
create_context_tpch(registrations: Vec<(string, string)>)? and then writing the 
vec in the test functions 



##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1297,6 +1298,32 @@ pub async fn from_substrait_rex(
                         outer_ref_columns,
                     })))
                 }
+                SubqueryType::SetPredicate(predicate) => {
+                    match predicate.predicate_op() {
+                        // exist
+                        PredicateOp::Exists => {
+                            let relations = &predicate.tuples;

Review Comment:
   super nit:
   ```suggestion
                               let relation = &predicate.tuples;
   ```



##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1297,6 +1298,32 @@ pub async fn from_substrait_rex(
                         outer_ref_columns,
                     })))
                 }
+                SubqueryType::SetPredicate(predicate) => {
+                    match predicate.predicate_op() {
+                        // exist
+                        PredicateOp::Exists => {
+                            let relations = &predicate.tuples;
+                            let plan = from_substrait_rel(
+                                ctx,
+                                &relations.clone().unwrap_or_default(),
+                                extensions,
+                            )
+                            .await?;
+                            let outer_ref_columns = plan.all_out_ref_exprs();
+                            Ok(Arc::new(Expr::Exists(Exists::new(
+                                Subquery {
+                                    subquery: Arc::new(plan),
+                                    outer_ref_columns,
+                                },
+                                false,
+                            ))))
+                        }
+                        other_type => Err(DataFusionError::Substrait(format!(
+                            "unimplemented type {:?} for set predicate",
+                            other_type
+                        ))),

Review Comment:
   would 
   ```suggestion
                           other_type => substrait_datafusion_err!(
                               "unimplemented type {:?} for set predicate",
                               other_type
                           ),
   ```
   work here as well?



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