Skip to content
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

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

carlyeks
Copy link
Contributor

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.

Copy link
Member

@EpsilonPrime EpsilonPrime left a 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.

@vbarua vbarua self-requested a review October 23, 2023 22:16
Copy link
Member

@vbarua vbarua left a 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.

@vbarua vbarua force-pushed the switch-expression branch from e3bde43 to 8480f07 Compare November 3, 2023 17:06
b.ifClause(b.equal(b.fieldReference(commonTable, 0), b.i32(10)), b.i32(2))),
b.i32(3)),
expression);
}
Copy link
Member

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.

Copy link
Member

@vbarua vbarua left a 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.

@vbarua vbarua merged commit b938573 into substrait-io:main Nov 3, 2023
ajegou pushed a commit to ajegou/substrait-java that referenced this pull request Mar 29, 2024
)

* feat: added or util to builder

---------

Co-authored-by: Victor Barua <victor.barua@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants