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: Enable adding missing equals to bool fields as filters #66252

Merged
merged 8 commits into from
Dec 15, 2020
Merged

SQL: Enable adding missing equals to bool fields as filters #66252

merged 8 commits into from
Dec 15, 2020

Conversation

bpintea
Copy link
Contributor

@bpintea bpintea commented Dec 14, 2020

This PR adds QL's AnalyzerRules.AddMissingEqualsToBoolField to SQL's analyser as well.

Closes #65579.

@bpintea
Enable the AddMissingEqualsToBoolField rule in SQL
b39016f 
This will enable QL's AnalyzerRules.AddMissingEqualsToBoolField to SQL's
analyzer as well.
@bpintea bpintea changed the title Enable adding missing equals to bool fields in SQL SQL: Enable adding missing equals to bool fields as filters Dec 14, 2020
@bpintea bpintea marked this pull request as ready for review December 14, 2020 15:46
@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Dec 14, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@bpintea bpintea requested review from costin, palesz, astefan and matriv and removed request for costin December 14, 2020 15:46
costin
costin approved these changes Dec 14, 2020
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

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/analyzer/AnalyzerRules.java
@@ -39,7 +40,7 @@ protected LogicalPlan rule(Filter filter) {
}

private Expression replaceRawBoolFieldWithEquals(Expression e) {
if (e instanceof FieldAttribute) {
if (e instanceof FieldAttribute && e.dataType() == BOOLEAN) {
Copy link
Member

Choose a reason for hiding this comment

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

The rule could check whether the Filter is resolved before applying the equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. Thanks, fixed.

@costin costin added v7.10.2 and removed v7.10.2 labels Dec 14, 2020
@bpintea
Fix: check type resolution before type's val
a5a6189 
Make sure a fields's type is available before checking it's value.
palesz
palesz reviewed Dec 14, 2020
View reviewed changes
Copy link
Contributor

@palesz palesz left a comment

Choose a reason for hiding this comment

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

LGTM.

So the outcome for SQL: Before all these queries below failed with exception during the query translation, while after this PR the first one will succeed.

SELECT * FROM test WHERE bool
SELECT * FROM test WHERE int
SELECT * FROM test WHERE str

@bpintea
Copy link
Contributor Author

bpintea commented Dec 14, 2020

So the outcome for SQL: Before all these queries below failed with exception during the query translation, while after this PR the first one will succeed.

That's correct.

@bpintea
Rework the way resolution is checked
6b2a6ee 
Check on filter's resolution part of the rule() method, before any other
change is applied.
@bpintea
Copy link
Contributor Author

bpintea commented Dec 15, 2020

@elasticmachine update branch

matriv
matriv reviewed Dec 15, 2020
View reviewed changes
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.

Left a comment.

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java Show resolved Hide resolved
@bpintea bpintea mentioned this pull request Dec 15, 2020
Merged
@bpintea
Add test with nested bool
5f24d32 
Add one SQL test with nested "free-floating" bool that got assigned a
true literal.
@bpintea
Merge branch 'enh/add_missing_eq_to_bool' of github.com:bpintea/elast…
28f79e4 
…icsearch into enh/add_missing_eq_to_bool
@bpintea
Copy link
Contributor Author

bpintea commented Dec 15, 2020

@elasticmachine update branch

@bpintea
Style fix
076d526 
- minor style fix.
matriv
matriv approved these changes Dec 15, 2020
View reviewed changes
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, thx!

@bpintea bpintea merged commit ea137f9 into elastic:master Dec 15, 2020
@bpintea bpintea deleted the enh/add_missing_eq_to_bool branch December 15, 2020 14:27
bpintea added a commit to bpintea/elasticsearch that referenced this pull request Dec 15, 2020
@bpintea
SQL: Enable adding missing equals to bool fields as filters (elastic#…
e1786d7 
…66252)

* Enable the AddMissingEqualsToBoolField rule in SQL

This will enable QL's AnalyzerRules.AddMissingEqualsToBoolField to SQL's
analyzer as well.

(cherry picked from commit ea137f9)
bpintea added a commit that referenced this pull request Dec 15, 2020
@bpintea
SQL: Enable adding missing equals to bool fields as filters (#66252) (#…
2933621 
…66355)

* Enable the AddMissingEqualsToBoolField rule in SQL

This will enable QL's AnalyzerRules.AddMissingEqualsToBoolField to SQL's
analyzer as well.

(cherry picked from commit ea137f9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@palesz palesz palesz left review comments

@matriv matriv matriv approved these changes

@costin costin costin approved these changes

@astefan astefan Awaiting requested review from astefan

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team v7.11.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SQL: enable adding missing equals to bool fields
6 participants
@bpintea @elasticmachine @costin @palesz @matriv @jakelandis