Improve error message in painless when parameter must be a constant but isn't #82638
Conversation
elasticsearchmachine
added
v8.1.0
external-contributor
stu-elastic
added
the
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -34,5 +34,31 @@ public static char StringTochar(final String value) { | |||
| return value.charAt(0); | |||
| } | |||
|
|
|||
| public static String toOrdinal(int number) { | |||
There was a problem hiding this comment.
Just the number is fine, no need to spend lines on adding the ordinal suffix.
| "all arguments to [" + javaMethod.getName() + "] must be constant but the [" + (i + 1) + "] argument isn't" | ||
| ) | ||
| ); | ||
| // offering the symbol name in error message (CastNode was evolved from ESymbol) |
There was a problem hiding this comment.
Does this handle cases when there are multiple arguments that should be constants?
|
@stu-elastic Thanks for your feedback! I've addressed to your comments in 84e6198 . |
| "all arguments to [" + javaMethod.getName() + "] must be constant but the [" + (i + 1) + "] argument isn't" | ||
| ) | ||
| ); | ||
| // offering the symbol name in error message (CastNode was evolved from ESymbol) |
There was a problem hiding this comment.
CastNode was evolved from ESymbol
What does this mean?
| Locale.ROOT, | ||
| "All arguments of the [%s] method must be constants, but the following arguments are not: %s", | ||
| javaMethod.getName(), | ||
| argumentsShouldBeConstants |
There was a problem hiding this comment.
The stream and Tuple use is unnecessary, why not build the error directly?
There was a problem hiding this comment.
My willingness is to print out all the errors at once. I will rollback this.
| ) | ||
| ); | ||
| // offering the symbol name in error message (CastNode was evolved from ESymbol) | ||
| String argumentName = argNode instanceof CastNode |
There was a problem hiding this comment.
How does this handle the case of 2 + x as an argument?
There was a problem hiding this comment.
It can't handle the 2 + x case yet, so if we need to handle this more complex case, I'm going to write a special method to build the original expression from Node.
There was a problem hiding this comment.
I see that the argNode has a location field with some information about the original expression, but it's missing the length. It would be much simpler to use this location field (no need to traverse the argNode's data structure)
| ); | ||
| // offering the symbol name in error message (CastNode was evolved from ESymbol) | ||
| String argumentName = argNode instanceof CastNode | ||
| ? ((CastNode) argNode).getChildNode().getDecoration(IRDName.class).getValue() |
There was a problem hiding this comment.
If we go with this code (which is not clear at the moment), getDecorationValue should be used to avoid the potential null dereference.
There was a problem hiding this comment.
OK, I will use getDecorationValue.
|
Hi, I'm going to close this PR as there's been no update in a year. Feel free to reopen it when the conflicts are resolved and the comments have been addressed. |
The pull request is related to #68324 .
I made the following changes:
ESymbol) name to error message