[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
Blizzara
force-pushed
the
avo/stronger-testing
branch
from
61faef5 to
623ee12
Compare
Blizzara
force-pushed
the
avo/stronger-testing
branch
from
623ee12 to
e374c85
Compare
| @@ -312,7 +312,7 @@ void scalarSubquery() { | |||
| Stream.of( | |||
| Expression.ScalarSubquery.builder() | |||
| .input(relWithEnhancement) | |||
| .type(TypeCreator.REQUIRED.struct(TypeCreator.REQUIRED.I64)) | |||
| .type(TypeCreator.REQUIRED.I64) | |||
There was a problem hiding this comment.
not 100% sure about this, is it actually meant to return a struct type? given it's scalar that seems a bit weird
No description provided.