SQL: Refactor args verification of In & conditionals #40916
Conversation
- Remove superfluous methods that are already defined in superclasses. - Improve tests for null folding on conditionals
Move verification of arguments for Conditional functions and In from `Verifier` to the `resolveType()` method of the functions.
|
Pinging @elastic/es-search |
costin
left a comment
costin
left a comment
There was a problem hiding this comment.
LGTM - I'm curious though why the ParsingException is not thrown anymore (and whether the exception hierarchy won't have side-effects).
|
|
||
| // Distinct isn't supported | ||
| ParsingException e = expectThrows(ParsingException.class, () -> | ||
| SqlIllegalArgumentException e = expectThrows(SqlIllegalArgumentException.class, () -> |
There was a problem hiding this comment.
Why is the ParsingException not thrown anymore?
There was a problem hiding this comment.
Thanks for catching that! the default def() method to register the functions is catching IllegalArgumentException and throwing ParsingException, so now some important info (like line number/column & function name is missing). Fixing that.
|
@elasticmachine run elasticsearch-ci/2 |
Move verification of arguments for Conditional functions and IN from `Verifier` to the `resolveType()` method of the functions. (cherry picked from commit 241644a)
astefan
left a comment
astefan
left a comment
There was a problem hiding this comment.
LGTM. Left few minor comments.
| public static final String NEW_LINE = "\n"; | ||
| public static final String SQL_WILDCARD = "%"; | ||
|
|
||
| private static final String[] INTEGER_ORDINALS = new String[] { "th", "st", "nd", "rd", "th", "th", "th", "th", "th", "th" }; |
There was a problem hiding this comment.
What's the first th for? 10th, 20th, 30th...?
| error("SELECT 1 IN ('foo', 'bar')")); | ||
| } | ||
|
|
||
| public void testInNestedWithDifferentDataTypesFromLeftValue_SelectClause() { |
There was a problem hiding this comment.
Why removing all these tests?
There was a problem hiding this comment.
Because this error was caught in the Verifier previously, by traversing the plan tree.
Now the check has been moved to the In function itself so there's no need to test ORDER BY, WHERE, etc...
| public void testInWithDifferentDataTypes_SelectClause() { | ||
| assertEquals("1:17: expected data type [integer], value provided is of type [keyword]", | ||
| public void testInWithDifferentDataTypes() { | ||
| assertEquals("1:8: 2nd argument of [1 IN (2, '3', 4)] must be [integer], found value ['3'] type [keyword]", |
There was a problem hiding this comment.
If there will be a query with IN where there are 100 values in its list, won't the error message be hard to follow?
There was a problem hiding this comment.
Could be, yes, because we now point to the begining of the IN function rather than the offending argument. But currently this how we address those type of errors for all functions and I opted of consistency.
Moreover, now it says: found value ['3'], for example, which the user can use to search within the IN list and find it quickly.
…e-unsafe-publication * elastic/master: Update contributing docs to reflect JDK 11 (elastic#40955) Docs: Simplifying setup by using module configuration variant syntax (elastic#40879) Unmute CreateIndexIT testCreateAndDeleteIndexConcurrently CI failures (elastic#40960) Revert "Short-circuit rebalancing when disabled (elastic#40942)" Mute EnableAllocationShortCircuitTests SQL: Refactor args verification of In & conditionals (elastic#40916) Avoid sharing source directories as it breaks intellij (elastic#40877) Short-circuit rebalancing when disabled (elastic#40942) SQL: Prefer resultSets over exceptions in metadata (elastic#40641) Mute ClusterPrivilegeTests.testThatSnapshotAndRestore Fix Race in AsyncTwoPhaseIndexerTests.testStateMachine (elastic#40947) Relax Overly Strict Assertion in TransportShardBulkAction (elastic#40940)
Move verification of arguments for Conditional functions and IN from `Verifier` to the `resolveType()` method of the functions.
5 participants
Move verification of arguments for Conditional functions and In
from
Verifierto theresolveType()method of the functions.Will be merged on top of: #40909