[CALCITE-7724] SqlUtil#lookupSubjectRoutines rejects a valid operator… - #5187
Open
sbroeder wants to merge 2 commits into
Open
[CALCITE-7724] SqlUtil#lookupSubjectRoutines rejects a valid operator…#5187sbroeder wants to merge 2 commits into
sbroeder wants to merge 2 commits into
Conversation
… when getFunctionKind() remaps its kind and 2+ candidates share a name filterOperatorRoutinesByKind's "fourth pass" only runs once at least two candidates survive the earlier passes (a single surviving candidate short-circuits before this pass). It compares candidate.getKind().getFunctionKind() against the call's own, already-bound SqlKind - but that comparison applies getFunctionKind()'s remapping (introduced for this method) to the candidate side only, not to the requested side. For any SqlKind that getFunctionKind() maps to something else - POSITION and the newly-dedicated CHAR_LENGTH both map to OTHER_FUNCTION, along with ~90 others in that switch - this asymmetry means an operator can fail to match even itself, once a second candidate for the same name is present (e.g. because two chained operator tables both contribute an entry for it). The call is rejected outright with "No match found for function signature ...", even though exactly the intended operator was available. Fix: map both sides of the comparison through getFunctionKind() before comparing, matching normal-case comparisons for kinds it doesn't remap. Added a regression test (chaining the standard operator table with itself to force the two-candidate precondition, the minimal way to reach the buggy pass) and verified the entire SqlValidatorTest suite (589 tests) still passes with the fix.
mihaibudiu
requested changes
Aug 18, 2026
|
|
||
| private static Iterator<SqlOperator> filterOperatorRoutinesByKind( | ||
| Iterator<SqlOperator> routines, final SqlKind sqlKind) { | ||
| // Map both sides through getFunctionKind() so a candidate can still match a call |
Contributor
There was a problem hiding this comment.
this comment could be shorter
| @@ -1043,6 +1025,43 @@ void testDyadicCollateOperator() { | |||
| .fails("Parameters must be of the same type"); | |||
| } | |||
|
|
|||
| /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7724"> | |||
Contributor
There was a problem hiding this comment.
this comment is too long; please do not submit useless comments, they will take time from reviewers now and forever when they will be read
| * spurious "No match found for function signature" validation error for an otherwise | ||
| * perfectly valid call. */ | ||
| @Test void testFunctionKindMismatchWithDuplicateOperatorTableEntry() { | ||
| // Chaining the standard operator table with itself is a minimal way to force two |
Contributor
There was a problem hiding this comment.
Enough to say that each function will appear twice
mihaibudiu
approved these changes
Aug 18, 2026
mihaibudiu
left a comment
mihaibudiu
left a comment
Contributor
There was a problem hiding this comment.
This is better. But at least Claude writes some horrible horrible comments. I highly recommend never submitting them as they are.
| * SqlKind is remapped by SqlKind#getFunctionKind() and two operator-table entries | ||
| * resolve to it</a>. | ||
| * | ||
| * <p>The kind-based fourth pass in {@code filterOperatorRoutinesByKind} maps only |
Contributor
There was a problem hiding this comment.
I am not sure how useful this paragraph is.
Contributor
Author
There was a problem hiding this comment.
I've tried to keep it short and be clear. Two competing goals. Are you okay with the comments as they are or do you have a preferred change?
|
Contributor
|
I have approved |
mihaibudiu
added
the
LGTM-will-merge-soon
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
… when getFunctionKind() remaps its kind and 2+ candidates share a name
filterOperatorRoutinesByKind's "fourth pass" only runs once at least two candidates survive the earlier passes (a single surviving candidate short-circuits before this pass). It compares candidate.getKind().getFunctionKind() against the call's own, already-bound SqlKind - but that comparison applies getFunctionKind()'s remapping (introduced for this method) to the candidate side only, not to the requested side.
For any SqlKind that getFunctionKind() maps to something else - POSITION and the newly-dedicated CHAR_LENGTH both map to OTHER_FUNCTION, along with ~90 others in that switch - this asymmetry means an operator can fail to match even itself, once a second candidate for the same name is present (e.g. because two chained operator tables both contribute an entry for it). The call is rejected outright with "No match found for function signature ...", even though exactly the intended operator was available.
Jira Link
CALCITE-7724
Changes Proposed