Skip to content

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