[SQL] Fix bug in compilation of queries with interned strings #5392
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a compiler crash when processing SQL queries involving interned strings, specifically addressing an assertion error where Some() was incorrectly applied to nullable InternedString expressions. The fix recognizes that interned strings are inherently nullable and should not be wrapped in Some. Additionally, the PR updates deprecated Calcite API calls to ensure compatibility with newer Calcite versions.
- Added logic to prevent wrapping
InternedStringtypes inSomeexpressions since they are already option values - Refactored Calcite type system configuration to replace deprecated methods with
getMaxPrecisionandgetMaxScaleoverrides - Added test case validating LEFT JOIN behavior with interned string columns
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| InternTests.java | Adds new test case for LEFT JOIN operations with interned strings to validate the bug fix |
| InternInner.java | Implements the core fix by overriding postorder for DBSPSomeExpression to skip wrapping interned types |
| SqlToRelCompiler.java | Updates Calcite API usage by replacing deprecated numeric precision/scale methods with type-specific overrides |
...-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/intern/InternInner.java
Show resolved
Hide resolved
...r/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/SqlToRelCompiler.java
Show resolved
Hide resolved
ryzhyk
approved these changes
Jan 7, 2026
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: Mihai Budiu <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this fix the compiler would crash with an assertion error:
The reason for this is that both nullable and non-nullable strings are represented by nullable InternedString objects, so the nullability of the data changes. The fix is to avoid wrapping in Some any InternedString - they are already option values.
I have also changed a call to a Calcite API which has been modified and will no longer compile on newer versions of Calcite.