[wip] fix: add proto roundtrips for Spark tests and fix issues it surfaces #315
Conversation
| // count only needs to be set when it is not -1 | ||
| builder.count(rel.getCount()); | ||
| } | ||
| var builder = Fetch.builder().input(input).offset(rel.getOffset()).count(rel.getCount()); |
There was a problem hiding this comment.
while the idea of not setting count if it's -1 is fine, this makes roundtrip tests fail if count is set in the pojo. Alternative fix is to ensure in the pojo it's never set if -1.
| @@ -131,7 +131,7 @@ class ToSubstraitRel extends AbstractLogicalPlanVisitor with Logging { | |||
| val aggregates = collectAggregates(actualResultExprs, aggExprToOutputOrdinal) | |||
| val aggOutputMap = aggregates.zipWithIndex.map { | |||
| case (e, i) => | |||
| AttributeReference(s"agg_func_$i", e.dataType)() -> e | |||
| AttributeReference(s"agg_func_$i", e.dataType, nullable = e.nullable)() -> e | |||
There was a problem hiding this comment.
these were causing wrong nullability for the type in the created pojos. I don't think that type field is used anywhere so it didn't cause harm, but still failed roundtrip tests as the type isn't written in proto and then it got correctly evaluated from other fields on read.
| val windowExpressions = window.windowExpressions | ||
| .flatMap(expr => expr.collect { case w: WindowExpression => w }) | ||
| .map(fromWindowCall(_, window.child.output)) | ||
| .asJava |
There was a problem hiding this comment.
the previous code would fail if there was a column like Alias(Alias(WindowExpression(..))), this catches those. It doesn't explicitly fail if there's some other wrappers for WindowExpressions than Alias, but I hope that's not the case in valid Spark plans
No description provided.