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.
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
SQL: Implement data type verification for conditionals #35916
Conversation
Add special verifier rule to check that the arguments of conditional functions are of the same or compatible types. This way the user gets a descriptive error message with line number and column indicating where is the offending argument. Closes: elastic#35907
Pinging @elastic/es-search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
e.forEachUp((ConditionalFunction greatest) -> { | ||
int idx = 0; | ||
DataType dt = DataType.NULL; | ||
for (; idx < greatest.children().size(); idx++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a one line comment here about this loop and the next one? (iterating until the first non-null value is found then continue and check for types equality)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some styling comments.
@@ -282,14 +284,13 @@ private static Failure fail(Node<?> source, String message, Object... args) { | |||
*/ | |||
private static boolean checkGroupBy(LogicalPlan p, Set<Failure> localFailures, | |||
Map<String, Function> resolvedFunctions, Set<LogicalPlan> groupingFailures) { | |||
return checkGroupByAgg(p, localFailures, groupingFailures, resolvedFunctions) | |||
&& checkGroupByOrder(p, localFailures, groupingFailures, resolvedFunctions) | |||
return checkGroupByAgg(p, localFailures, resolvedFunctions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the removed parameters were not needed inside the methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
// Find the data type of the first non-null argument | ||
for (; idx < cf.children().size(); idx++) { | ||
DataType childDT = cf.children().get(idx).dataType(); | ||
if (childDT != DataType.NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opt for Expressions.isNull
as it might catch null literals with a different type than NULL
.
} | ||
} | ||
// Compare the data type of the rest of the arguments with the data type of first non-null argument | ||
for (; idx < cf.children().size(); idx++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the loops should be folded into one for-loop with an if/else:
DataType dt = DataType.NULL;
for (Expression child : cf.children()) {
if (dt == NULL) {
if (Expressions.isNull(child) == false) {
dt = child.dataType();
}
} else {
if (areTypesCompatible(...)) {
...
}
}
}
Add special verifier rule to check that the arguments of conditional functions are of the same or compatible types. This way the user gets a descriptive error message with line number and column indicating where is the offending argument. Closes: #35907
Backported to |
Add special verifier rule to check that the arguments of conditional
functions are of the same or compatible types. This way the user gets
a descriptive error message with line number and column indicating
where is the offending argument.
Closes: #35907