Change API to return LogicalPlan instead of DataFrame #35
Conversation
| left.join(right, join_type, &left_cols, &right_cols, None) | ||
| let left_cols: Vec<Column> = pairs.iter().map(|(l, _)| l.clone()).collect(); | ||
| let right_cols: Vec<Column> = pairs.iter().map(|(_, r)| r.clone()).collect(); | ||
| left.join(&right, join_type, (left_cols, right_cols), None)? |
There was a problem hiding this comment.
Is the last parameter (filter) that's set to None the same field semantically as Substrait JoinRel's post_join_filter?
If not, please ignore the comment.
If so, should we check if the JoinRel has post_join_filter? And if so, parse it?
There was a problem hiding this comment.
Yes, I think those probably are the same concept. It would be good to implement this in a separate PR.
waynexia
left a comment
waynexia
left a comment
There was a problem hiding this comment.
Looks good to me. By the way, I prefer to remove optimize() calls later as it is out of the scope of serializing/deserializing. The downstream consumer can do optimization by themselves.
Yes, I agree with that. |
3 participants
As suggested by @nseekhao, given that
to_substrait_rel()takesLogicalPlanand returns SubstraitRel, it would be better iffrom_subtrait_rel()takes SubstraitReland returnsLogicalPlanrather thanDataFrame.