fix(isthmus): is_distinct_from tests to cover rewrite#628
fix(isthmus): is_distinct_from tests to cover rewrite#628asuriy wants to merge 1 commit intosubstrait-io:mainfrom
Conversation
There was a problem hiding this comment.
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.
Addressing comment here on logical rewrite: https://github.com/substrait-io/substrait-java/pull/597/files/b792247416f9a864a99ba3842c1219c3a083d7c4#r2578056630