Skip to content

feat(isthmus): introduce SubstraitSqlToCalcite and SubstraitStatementParser #474

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 19, 2025
Merged

feat(isthmus): introduce SubstraitSqlToCalcite and SubstraitStatementParser #474

merged 7 commits into from
Aug 19, 2025

Conversation

vbarua
Copy link
Member

@vbarua vbarua commented Aug 11, 2025

No description provided.

@vbarua vbarua force-pushed the vbarua/substrait-sql-to-calcite branch from e887474 to cc5da77 Compare August 11, 2025 23:02
@vbarua vbarua marked this pull request as ready for review August 11, 2025 23:05
@vbarua
Copy link
Member Author

vbarua commented Aug 11, 2025

This PR continues the work of #362 in splitting the SQL -> Calcite and Calcite -> Substrait API into modulate components.

@vbarua vbarua requested a review from nielspardon August 11, 2025 23:07
@vbarua vbarua mentioned this pull request Aug 11, 2025
Merged
@vbarua vbarua force-pushed the vbarua/substrait-sql-to-calcite branch from d0e0aa4 to 41670c2 Compare August 15, 2025 05:00
nielspardon
nielspardon approved these changes Aug 15, 2025
View reviewed changes
Copy link
Member

@nielspardon nielspardon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with the few minor documentation fixes please see comment below

nielspardon
nielspardon reviewed Aug 15, 2025
View reviewed changes
isthmus/src/test/java/io/substrait/isthmus/NameRoundtripTest.java
Comment on lines +28 to +30
org.apache.calcite.rel.RelRoot calciteRelRoot1 =
SubstraitSqlToCalcite.convertQuery(query, catalogReader);
assertEquals(expectedNames, calciteRelRoot1.validatedRowType.getFieldNames());
Copy link
Member

@nielspardon nielspardon Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This change I don't fully follow. How is it an improvement that we are testing SQL to Calcite before we test SQL to Substrait? Shouldn't it be sufficient if we test SQL to Substrait since that is our non-experimentation public API? Shouldn't it be obvious that Calcite's SQL To RelNode conversion preserves the names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't it be obvious that Calcite's SQL To RelNode conversion preserves the names?

You would think so, but if you get rid of the quoting the (current) parser settings will uppercase all the identifiers. This was checked indirectly, but because I had access to a convenient converter from Sql to Calcite now I felt that adding a direct check for this would be helpful.

nielspardon
nielspardon reviewed Aug 15, 2025
View reviewed changes
nielspardon
nielspardon requested changes Aug 15, 2025
View reviewed changes
Copy link
Member

@nielspardon nielspardon left a comment

Choose a reason for hiding this comment

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

sorry, approved maybe a little to soon. can you enlighten me on the reasoning behind the changes to NameRoundtripTest and PlanTestBase. What are we gaining by breaking these into multiple steps instead of using the SqlToSubstrait wrapper?

@ZorinAnton ZorinAnton mentioned this pull request Aug 15, 2025
Open
@vbarua vbarua force-pushed the vbarua/substrait-sql-to-calcite branch from 2797435 to c48bcce Compare August 15, 2025 16:09
@nielspardon nielspardon self-requested a review August 18, 2025 13:06
@vbarua vbarua merged commit 8906eb8 into main Aug 19, 2025
11 of 12 checks passed
@vbarua vbarua deleted the vbarua/substrait-sql-to-calcite branch August 19, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielspardon nielspardon nielspardon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vbarua @nielspardon