-
Notifications
You must be signed in to change notification settings - Fork 74
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
fix: map switch expression to a Calcite CASE statement #189
Conversation
bd5e22c
to
e3bde43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid to me. I'll let others have a chance to take a look at it though.
isthmus/src/test/java/io/substrait/isthmus/SubstraitRelNodeConverterTest.java
Outdated
Show resolved
Hide resolved
isthmus/src/test/java/io/substrait/isthmus/ExpressionConvertabilityTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just some small comments about tests.
e3bde43
to
8480f07
Compare
b.ifClause(b.equal(b.fieldReference(commonTable, 0), b.i32(10)), b.i32(2))), | ||
b.i32(3)), | ||
expression); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests were creating a full plan and converting it, but we only actually want/need to verify the expression conversion.
Removed the plan related noise by converting the expression directly. Also used a bunch of our nice utils (including one I added) to make the tests more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core implementation looks good. I went ahead and updated some of the tests, mostly to simplify them.
The switch expression POJO in substrait-java doesn't contain the match expression, so I've updated the definition to include that.
To map into Calcite, I construct a CASE statement. This ends up being translated into an IfThen expression when round-tripping Substrait into Calcite.