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: Introduce NotEquals node to simplify expressions #35234

Merged
merged 5 commits into from Nov 5, 2018
Merged

SQL: Introduce NotEquals node to simplify expressions #35234

merged 5 commits into from Nov 5, 2018

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Nov 4, 2018

Add NotEquals node in parser to simplify expressions so that != is
no longer translated internally to NOT( = )

Closes: #35210
Fixes: #35233

@matriv
SQL: Introduce NotEquals node to simplify expressions
c2f0b3e 
Add NotEquals node in parser to simplify expressions so that <value1> != <value2> is
no longer translated internally to NOT(<value1> = <value2>)

Closes: elastic#35210
Fixes: elastic#35233
@matriv matriv requested review from costin and astefan November 4, 2018 11:16
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

costin
costin reviewed Nov 5, 2018
View reviewed changes
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java Outdated
@@ -585,7 +585,11 @@ private static Query translateQuery(BinaryComparison bc) {
name = fa.exactAttribute().name();
}
}
return new TermQuery(loc, name, value);
Query query= new TermQuery(loc, name, value);
Copy link
Member

Choose a reason for hiding this comment

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

formatting : query=

costin
costin approved these changes Nov 5, 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.

LGTM - I wonder why though the != was not failing before - wasn't the terms query properly wrapped in a negation?

...earch/xpack/sql/expression/predicate/operator/comparison/BinaryComparisonProcessorTests.java
@@ -11,12 +11,6 @@
import org.elasticsearch.xpack.sql.expression.Literal;
import org.elasticsearch.xpack.sql.expression.function.scalar.Processors;
import org.elasticsearch.xpack.sql.expression.gen.processor.ConstantProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
Copy link
Member

Choose a reason for hiding this comment

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

? These classes are still used so why are the imports removed?

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 in the same package ;)

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java
@@ -536,16 +537,15 @@ protected QueryTranslation asQuery(BinaryComparison bc, boolean onAggs) {
//
// Agg context means HAVING -> PipelineAggs
//
ScriptTemplate script = bc.asScript();
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why it got moved down? Did it fail when used against a FieldAttribute (which suggest a bug in the pipeline/querytranslator)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no bug there, I just thought that it's better to called the asScript() when actually needed and not beforehand. If you have any objection I can revert.

@matriv
Copy link
Contributor Author

matriv commented Nov 5, 2018

@costin the fix is here: https://github.com/elastic/elasticsearch/pull/35234/files#diff-c96a7f73e2c1f3d5fbdf8c934f8f0aa3R58

astefan
astefan approved these changes Nov 5, 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. Left just two minor comments.

x-pack/plugin/sql/qa/src/main/resources/select.csv-spec Outdated
SELECT languages = 2, NOT languages = 2, languages != 2, NOT languages != 2, languages != null, NOT null != languages
FROM "test_emp" WHERE emp_no IN(10018, 10019, 10020) ORDER BY emp_no ;

2
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these 3 lines accepted by the test class? I'm assuming you added these to show which languages values you are testing against. If so, why not showing the languages column as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no this was a leftover, I'll remove them but I will merge #35239 first which fixes and enables existing tests.

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/Comparisons.java Outdated
@@ -19,6 +19,11 @@ public static Boolean eq(Object l, Object r) {
return i == null ? null : i.intValue() == 0;
}

static Boolean neq(Object l, Object r) {
Integer i = compare(l, r);
return i == null? null : i.intValue() != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

return i == null? null : i.intValue() != 0; just add a whitespace: return i == null ? null : i.intValue() != 0;. If the PR is held back by this picky comment, you can ignore it and merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries, I'll fix, need to merge once the select-csv.spec is fixed anyway.

@matriv matriv merged commit 9ac7af6 into elastic:master Nov 5, 2018
@matriv matriv deleted the mt/fix-35210 branch November 5, 2018 21:23
matriv pushed a commit that referenced this pull request Nov 5, 2018
@matriv
SQL: Introduce NotEquals node to simplify expressions (#35234)
1c7c6de 
Add NotEquals node in parser to simplify expressions so that <value1> != <value2> is
no longer translated internally to NOT(<value1> = <value2>)

Closes: #35210
Fixes: #35233
@matriv
Copy link
Contributor Author

matriv commented Nov 5, 2018

Backported to 6.x with 1c7c6de

matriv pushed a commit that referenced this pull request Nov 5, 2018
@matriv
SQL: Introduce NotEquals node to simplify expressions (#35234)
a21316b 
Add NotEquals node in parser to simplify expressions so that <value1> != <value2> is
no longer translated internally to NOT(<value1> = <value2>)

Closes: #35210
Fixes: #35233
@matriv
Copy link
Contributor Author

matriv commented Nov 5, 2018

Backported to 6.5 with a21316b

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

@costin costin costin approved these 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.

None yet
6 participants
@matriv @elasticmachine @costin @astefan @colings86 @jimczi