Implement TPCH substrait integration test, support tpch_4 and tpch_5 #11311
Conversation
|
not much to change for these two queries. |
alamb
changed the title
There was a problem hiding this comment.
Thank you @Lordworms -- I had one small suggestion, but I don't think it is nececssary
FYI @Blizzara
| \n Sort: FILENAME_PLACEHOLDER_0.o_orderpriority ASC NULLS LAST\ | ||
| \n Aggregate: groupBy=[[FILENAME_PLACEHOLDER_0.o_orderpriority]], aggr=[[count(Int64(1))]]\ | ||
| \n Projection: FILENAME_PLACEHOLDER_0.o_orderpriority\ | ||
| \n Filter: FILENAME_PLACEHOLDER_0.o_orderdate >= CAST(Utf8(\"1993-07-01\") AS Date32) AND FILENAME_PLACEHOLDER_0.o_orderdate < CAST(Utf8(\"1993-10-01\") AS Date32) AND EXISTS (<subquery>)\ |
| @@ -1297,6 +1297,32 @@ pub async fn from_substrait_rex( | |||
| outer_ref_columns, | |||
| }))) | |||
| } | |||
| SubqueryType::SetPredicate(predicate) => { | |||
| match predicate.predicate_op { | |||
There was a problem hiding this comment.
It looks like we could use https://docs.rs/substrait/0.35.0/substrait/proto/expression/subquery/struct.SetPredicate.html#method.predicate_op to match on PredicateOp rather than a constant
So lke
match predicate.predicate_op() {
PredicateOp::Exists => ...
other_type => ...
}There was a problem hiding this comment.
Sure, I'll fix it.
| match predicate.predicate_op() { | ||
| // exist | ||
| PredicateOp::Exists => { | ||
| let relations = &predicate.tuples; |
There was a problem hiding this comment.
super nit:
| let relations = &predicate.tuples; | |
| let relation = &predicate.tuples; |
| other_type => Err(DataFusionError::Substrait(format!( | ||
| "unimplemented type {:?} for set predicate", | ||
| other_type | ||
| ))), |
There was a problem hiding this comment.
would
| other_type => Err(DataFusionError::Substrait(format!( | |
| "unimplemented type {:?} for set predicate", | |
| other_type | |
| ))), | |
| other_type => substrait_datafusion_err!( | |
| "unimplemented type {:?} for set predicate", | |
| other_type | |
| ), |
work here as well?
| async fn create_context_tpch4() -> Result<SessionContext> { | ||
| let ctx = SessionContext::new(); | ||
|
|
||
| let registrations = vec
There was a problem hiding this comment.
Thanks @Lordworms and @Blizzara -- I implemented @Blizzara 's suggestions other than https://github.com/apache/datafusion/pull/11311/files#r1668128566 which I agree would be nicer, though we can keep it in a separate PR
|
🚀 |
…pache#11311) * Implement TPCH substrait integration teset, support tpch_4 and tpch_5 * optimize code * rename variable * Use error macro --------- Co-authored-by: Andrew Lamb <[email protected]>
…pache#11311) * Implement TPCH substrait integration teset, support tpch_4 and tpch_5 * optimize code * rename variable * Use error macro --------- Co-authored-by: Andrew Lamb <[email protected]>
Which issue does this PR close?
part of #10710
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?