-
Notifications
You must be signed in to change notification settings - Fork 93
feat: support Nested Lists #627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
95c27c9
4b42b10
9e99091
5b277c9
3ae2b62
0e4c6fd
5ad5fa1
925c9ef
b0a787d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
gord02 marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,94 @@ | ||||||
| package io.substrait.type.proto; | ||||||
|
|
||||||
| import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; | ||||||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||||||
|
|
||||||
| import io.substrait.TestBase; | ||||||
| import io.substrait.expression.Expression; | ||||||
| import io.substrait.expression.ImmutableExpression; | ||||||
| import org.junit.jupiter.api.Test; | ||||||
|
|
||||||
| class NestedListExpressionTest extends TestBase { | ||||||
| io.substrait.expression.Expression literalExpression = | ||||||
| Expression.BoolLiteral.builder().value(true).build(); | ||||||
| Expression.ScalarFunctionInvocation nonLiteralExpression = b.add(b.i32(7), b.i32(42)); | ||||||
|
|
||||||
| @Test | ||||||
| void DifferentTypedLiteralsNestedListTest() { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
minor suggestion: if we include the expected behaviour in the test name, it's easier to see at at glance what its testing. Similar suggestions for test below. |
||||||
| ImmutableExpression.NestedList.Builder builder = | ||||||
| Expression.NestedList.builder().addValues(literalExpression).addValues(b.i32(12)); | ||||||
| assertThrows(AssertionError.class, builder::build); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| void SameTypedLiteralsNestedListTest() { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Beyond the name suggestion for this test, it is a bit redundant because it doesn't really verify anything differently than your normal tests below do. |
||||||
| ImmutableExpression.NestedList.Builder builder = | ||||||
| Expression.NestedList.builder().addValues(nonLiteralExpression).addValues(b.i32(12)); | ||||||
| assertDoesNotThrow(builder::build); | ||||||
|
|
||||||
| io.substrait.relation.Project project = | ||||||
| io.substrait.relation.Project.builder() | ||||||
| .addExpressions(builder.build()) | ||||||
| .input(b.emptyScan()) | ||||||
| .build(); | ||||||
| verifyRoundTrip(project); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| void EmptyNestedListTest() { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ImmutableExpression.NestedList.Builder builder = Expression.NestedList.builder(); | ||||||
| assertThrows(AssertionError.class, builder::build); | ||||||
| } | ||||||
gord02 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| @Test | ||||||
| void literalNestedListTest() { | ||||||
| Expression.NestedList literalNestedList = | ||||||
| Expression.NestedList.builder() | ||||||
| .addValues(literalExpression) | ||||||
| .addValues(literalExpression) | ||||||
| .build(); | ||||||
|
|
||||||
| io.substrait.relation.Project project = | ||||||
| io.substrait.relation.Project.builder() | ||||||
| .addExpressions(literalNestedList) | ||||||
| .input(b.emptyScan()) | ||||||
| .build(); | ||||||
|
|
||||||
| verifyRoundTrip(project); | ||||||
gord02 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| void literalNullableNestedListTest() { | ||||||
| Expression.NestedList literalNestedList = | ||||||
| Expression.NestedList.builder() | ||||||
| .addValues(literalExpression) | ||||||
| .addValues(literalExpression) | ||||||
| .nullable(true) | ||||||
| .build(); | ||||||
|
|
||||||
| io.substrait.relation.Project project = | ||||||
| io.substrait.relation.Project.builder() | ||||||
| .addExpressions(literalNestedList) | ||||||
| .input(b.emptyScan()) | ||||||
| .build(); | ||||||
|
|
||||||
| verifyRoundTrip(project); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| void nonLiteralNestedListTest() { | ||||||
| Expression.NestedList nonLiteralNestedList = | ||||||
| Expression.NestedList.builder() | ||||||
| .addValues(nonLiteralExpression) | ||||||
| .addValues(nonLiteralExpression) | ||||||
| .build(); | ||||||
|
|
||||||
| io.substrait.relation.Project project = | ||||||
| io.substrait.relation.Project.builder() | ||||||
| .addExpressions(nonLiteralNestedList) | ||||||
| .input(b.emptyScan()) | ||||||
| .build(); | ||||||
|
|
||||||
| verifyRoundTrip(project); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package io.substrait.isthmus; | ||
|
|
||
| import org.apache.calcite.sql.SqlFunction; | ||
| import org.apache.calcite.sql.SqlFunctionCategory; | ||
| import org.apache.calcite.sql.SqlKind; | ||
| import org.apache.calcite.sql.type.ReturnTypes; | ||
|
|
||
| /** Substrait-specific extension function for the Expression Nested List type */ | ||
| public class NestedFunctions { | ||
|
|
||
| public static final SqlFunction NESTED_LIST = | ||
| new SqlFunction( | ||
| "nested_list", | ||
| SqlKind.OTHER_FUNCTION, | ||
| ReturnTypes.BOOLEAN, | ||
gord02 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| null, | ||
| null, | ||
| SqlFunctionCategory.USER_DEFINED_FUNCTION); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -179,6 +179,10 @@ public Rel visit(org.apache.calcite.rel.core.Project project) { | |||||||||
| .map(this::toExpression) | ||||||||||
| .collect(java.util.stream.Collectors.toList()); | ||||||||||
|
|
||||||||||
| // if there is no input fields, don’t put a remapping on it | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
nit
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor/pedantic: small rewording + avoiding extra whitespace
Suggested change
|
||||||||||
| if (project.getInput().getRowType().getFieldCount() == 0) { | ||||||||||
| return Project.builder().expressions(expressions).input(apply(project.getInput())).build(); | ||||||||||
| } | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change looks good, but is a bit out of place in this PR. From experimenting locally it seems that one of your tests fails locally with it because a pointless remap is introduced. I do think it's a good change, and we should keep it in this PR, but could you add an explicit test for this features along the lines of |
||||||||||
| // todo: eliminate excessive projects. This should be done by converting rexinputrefs to remaps. | ||||||||||
| return Project.builder() | ||||||||||
| .remap( | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package io.substrait.isthmus.expression; | ||
|
|
||
| import io.substrait.expression.Expression; | ||
| import io.substrait.isthmus.CallConverter; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.function.Function; | ||
| import java.util.stream.Collectors; | ||
| import org.apache.calcite.rex.RexCall; | ||
| import org.apache.calcite.rex.RexNode; | ||
|
|
||
| public class NestedExpressionConverter implements CallConverter { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its a reasonable choice not to have this be a nested class in CallConverters. That said, could you rename this to |
||
|
|
||
| public NestedExpressionConverter() {} | ||
|
|
||
| @Override | ||
| public Optional<Expression> convert( | ||
| RexCall call, Function<RexNode, Expression> topLevelConverter) { | ||
|
|
||
| if (!call.getOperator().getName().equals("nested_list")) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| List<Expression> values = | ||
| call.operands.stream().map(topLevelConverter).collect(Collectors.toList()); | ||
|
|
||
| return Optional.of( | ||
| Expression.NestedList.builder() | ||
| .nullable(call.getType().isNullable()) | ||
| .values(values) | ||
| .build()); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.