Skip to content
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: Scripting support for casting functions CAST and CONVERT #36640

Merged
merged 4 commits into from
Dec 17, 2018

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Dec 14, 2018

This PR fixes #36061. Up until this point, there was no scripting support for CAST and CONVERT and, as such, CAST and CONVERT cannot be used in GROUP BY and ORDER BY expressions (where Painless scripting is used).

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@astefan
Copy link
Contributor Author

astefan commented Dec 14, 2018

run the gradle build tests 2

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left a couple of comments, and also can we have an integ test with the cast used also in the HAVING?

// Casting
//
public static Object convert(Object value, String typeName) {
if (value == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's unnecessary, it's done internally in convert()

#
# Casting
#
def convert(Object, String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference to name it cast which is the main name for SQL, but convert is also good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on cast for me as well.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the implementation moving over to processScript instead.

@@ -74,6 +78,18 @@ protected Processor makeProcessor() {
return new CastProcessor(DataTypeConversion.conversionFor(from(), to()));
}

@Override
public ScriptTemplate asScript() {
ScriptTemplate fieldAsScript = asScript(field());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the solution is to use a slightly different script, simply override processScript (see its implementations for examples like MathFunction). This way you only need to define the string and not worry about the boiler-plate around passing the script further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With processScript we cannot set the dataType as parameter and we are forced to send it as String inline. Will leave it as is at the moment, as discussed.

#
# Casting
#
def convert(Object, String)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on cast for me as well.

@astefan
Copy link
Contributor Author

astefan commented Dec 14, 2018

@matriv when using HAVING I get the same error as the one here (with CAST) and the one here. Looking into it a bit - the Painless script seems incomplete, but if I add the casting part (which is missing) the error is still there unchanged. In the second scenario, the query isn't actually built.

@matriv
Copy link
Contributor

matriv commented Dec 17, 2018

@astefan then let's go ahead and merge this and check afterwards the HAVING case.

@astefan astefan merged commit b15f27f into elastic:master Dec 17, 2018
@astefan astefan deleted the 36061_fix branch December 17, 2018 11:37
@pcsanwald
Copy link
Contributor

closes #34557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL: Error in painless script generation for complex string functions
6 participants