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: Fix null handling for AND and OR in SELECT #35277

Merged
merged 2 commits into from
Nov 6, 2018

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Nov 5, 2018

Override process() in BinaryLogicProcessor which doesn't immediately
return null if left or right argument is null, which is the behaviour of
process() of the parent class BinaryProcessor.

Also, add more tests for AND and OR in SELECT clause with literal.

Fixes: #35240

Override `process()` in `BinaryLogicProcessor` which doesn't immediately
return null if left or right argument is null, which is the behaviour of
`process()` of the parent class `BinaryProcessor`.

Also, add more tests for `AND` and `OR` in SELECT clause with literal.

Fixes: elastic#35240
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@klobin
Copy link

klobin commented Nov 6, 2018

Can I take up this issue ?

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.

Left one minor comment

assertNull(new BinaryLogicProcessor(NULL, NULL, BinaryLogicProcessor.BinaryLogicOperation.AND).process(null));
}

private static Boolean randomBooleanOrNull() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this method, any of the randomFrom() methods could work?

Copy link
Contributor Author

@matriv matriv Nov 6, 2018

Choose a reason for hiding this comment

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

Sure, didn't think of that, fixed, thx!

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 Nov 6, 2018

retest this please

@costin
Copy link
Member

costin commented Nov 6, 2018

@klobin Thank you for your interest.
However note this is a PR, meaning code that's already addressing the initial problem.
Please take a look at our contributing guide for more information.

Thanks!

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

@matriv matriv merged commit 1be64a7 into elastic:master Nov 6, 2018
@matriv matriv deleted the mt/fix-35240 branch November 6, 2018 14:40
matriv pushed a commit that referenced this pull request Nov 6, 2018
Override `process()` in `BinaryLogicProcessor` which doesn't immediately
return null if left or right argument is null, which is the behaviour of
`process()` of the parent class `BinaryProcessor`.

Also, add more tests for `AND` and `OR` in SELECT clause with literal.

Fixes: #35240
@matriv
Copy link
Contributor Author

matriv commented Nov 6, 2018

Backported to 6.x with 68e0359

matriv pushed a commit that referenced this pull request Nov 6, 2018
Override `process()` in `BinaryLogicProcessor` which doesn't immediately
return null if left or right argument is null, which is the behaviour of
`process()` of the parent class `BinaryProcessor`.

Also, add more tests for `AND` and `OR` in SELECT clause with literal.

Fixes: #35240
@matriv
Copy link
Contributor Author

matriv commented Nov 6, 2018

Backported to 6.5 with b5ba739

matarrese added a commit to matarrese/elasticsearch that referenced this pull request Nov 6, 2018
…-agg

* master: (528 commits)
  Register Azure max_retries setting (elastic#35286)
  add version 6.4.4
  [Docs] Add painless context details for bucket_script (elastic#35142)
  Upgrade jline to 3.8.2 (elastic#35288)
  SQL: new SQL CLI logo (elastic#35261)
  Logger: Merge ESLoggerFactory into Loggers (elastic#35146)
  Docs: Add section about range query for range type (elastic#35222)
  [ILM] change remove-policy-from-index http method from DELETE to POST (elastic#35268)
  [CCR] Forgot missing return statement,
  SQL: Fix null handling for AND and OR in SELECT (elastic#35277)
  [TEST] Mute ChangePolicyForIndexIT#testChangePolicyForIndex
  Serialize ignore_throttled also to 6.6 after backport
  Check for java 11 in buildSrc (elastic#35260)
  [TEST] increase await timeout in RemoteClusterConnectionTests
  Add missing up-to-date configuration (elastic#35255)
  Adapt Lucene BWC version
  SQL: Introduce Coalesce function (elastic#35253)
  Upgrade to lucene-8.0.0-snapshot-31d7dfe6b1 (elastic#35224)
  Fix failing ICU tests (elastic#35207)
  Prevent throttled indices to be searched through wildcards by default (elastic#34354)
  ...
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants