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.

Sign up for GitHub

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

Jump to bottom

SQL: Fix null handling for IN => painless script #35124

Merged
merged 3 commits into from
Nov 2, 2018
Merged

SQL: Fix null handling for IN => painless script #35124

merged 3 commits into from
Nov 2, 2018

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Oct 31, 2018 •
edited
Loading

Include null literals when generating the painless script for IN expressions.
Previously, they were skipped, because of an issue that has been fixed with #35108.

Fixes: #35122

@matriv
SQL: Fix null handling for IN => painless script
c379800 
Include `null` literals when generating the painless script for `IN` expressions.
Previously, they were skipped, because of an issue that had been fixed with elastic#35108.

Fixes: elastic#35122
@matriv matriv requested review from costin and astefan October 31, 2018 10:51
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

astefan
astefan approved these changes Oct 31, 2018
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@matriv
Copy link
Contributor Author

matriv commented Oct 31, 2018

retest this please

costin
costin requested changes Oct 31, 2018
View reviewed changes
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.

The script generation should delegate to InternalSqlScriptUtils not replicate the code.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java Outdated
@@ -84,11 +84,12 @@ public boolean foldable() {

@Override
public Boolean fold() {
// Optimization for early return and Query folding to LocalExec
if (value.dataType() == DataType.NULL) {
return null;
}
if (list.size() == 1 && list.get(0).dataType() == DataType.NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

What does this line do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the same optimization to fold to LocalExec. I can merge the ifs

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java Outdated
ScriptTemplate rightScript = asScript((Expression) valueFromList);
sj.add(scriptPrefix + rightScript.template());
rightParams.add(rightScript.params());
if (valueFromList instanceof Expression) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this block ever executing considering the values is a list of folded expressions and thus values.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java Outdated
sj.add(scriptPrefix + rightScript.template());
rightParams.add(rightScript.params());
} else {
if (valueFromList instanceof String) {
Copy link
Member

Choose a reason for hiding this comment

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

The script should just delegate to InternalSqlScriptsUtils.in not duplicate the code inside the script.

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java Outdated
@@ -254,7 +255,8 @@ public void testTranslateInExpression_HavingClause_PainlessAndNullHandling() {
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
assertNull(translation.query);
AggFilter aggFilter = translation.aggFilter;
assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10 || params.a0==20 || params.a0==30)",
assertEquals("InternalSqlScriptUtils.nullSafeFilter(" +
Copy link
Member

Choose a reason for hiding this comment

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

This should be nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params) with the constants being sent as a params to the script

@costin
Copy link
Member

costin commented Oct 31, 2018

I realize this is impacted by #35055 but it makes the change set confusing. Could you please rebase this over that or potentially merge that first and the update this ticket.
It will be useful also in the future when looking back and what the changes actually were (looks to be the return of null ?).

Thanks

@matriv
Copy link
Contributor Author

matriv commented Oct 31, 2018

OK! Will merge the other PR first and adjust this based on the other.

@matriv
Copy link
Contributor Author

matriv commented Nov 1, 2018

@costin updated on top of the #35055 and the fix is just to remove the null filtering: https://github.com/elastic/elasticsearch/pull/35124/files#diff-c67de98b2f74bb15fb8cdcbea8ef573cL112

@matriv matriv merged commit 340363b into elastic:master Nov 2, 2018
@matriv matriv deleted the mt/fix-35122 branch November 2, 2018 09:38
matriv pushed a commit that referenced this pull request Nov 2, 2018
@matriv
SQL: Fix null handling for IN => painless script (#35124)
bdf5d05 
Include `null` literals when generating the painless script for `IN` expressions.
Previously, they were skipped, because of an issue that had been fixed with #35108.

Fixes: #35122
@matriv
Copy link
Contributor Author

matriv commented Nov 2, 2018

Backported to 6.x with bdf5d05

matriv pushed a commit that referenced this pull request Nov 2, 2018
@matriv
SQL: Fix null handling for IN => painless script (#35124)
7591555 
Include `null` literals when generating the painless script for `IN` expressions.
Previously, they were skipped, because of an issue that had been fixed with #35108.

Fixes: #35122
@matriv
Copy link
Contributor Author

matriv commented Nov 2, 2018

Backported to 6.5 with 7591555

@colings86 colings86 added v6.5.0 and removed v6.5.1 labels Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin costin costin requested changes

@astefan astefan astefan approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >bug v6.5.0 v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SQL: IN translated to painless has issue with null handling
5 participants
@matriv @elasticmachine @costin @astefan @colings86