Skip to content

Conversation

@asuriy
Copy link
Contributor

@asuriy asuriy commented Dec 2, 2025

Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced this is a worthwhile addition. What you have already is a valid test that Isthmus can convert an SQL query containing IS DISTINCT FROM into Substrait (and back again). The fact that Calcite rewrites this into a different (but logically equivalent) form doesn't really matter from the perspective of supporting this SQL input. Explicitly testing that Calcite has applied this rewriting just makes the test unnecessarily restrictive. Calcite behaviour might change in the future and it really doesn't matter to us as as long as the SQL can still be converted to/from Substrait.

I suspect that Victor's point might have been that you were not testing the function mapping you added for SqlStdOperatorTable.IS_DISTINCT_FROM. Calcite's rewriting means that mapping is not used (or tested). To explicitly test it you might have to craft a Calcite Rel structure as input to the Isthmus conversion of Calcite Rel to Substrait. Whether this level of testing is actually worth the effort, I am not sure.

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.

2 participants