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

Fix issue with CASE/IIF pre-calculating results #49553

Merged
merged 3 commits into from
Nov 26, 2019
Merged

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Nov 25, 2019

Previously, CaseProcessor was pre-calculating (called process())
on all the building elements of a CASE/IIF expression, not only the
conditions involved but also the results (if true/if false), as well as
the final else result. In case one of those results had an erroneous
calculation (e.g.: division by zero) this was executed and resulted in
an Exception to be thrown, even if this result was not used because of
the condition guarding it. e.g.:

SELECT CASE WHEN myField1 = 0 THEN NULL ELSE myField2 / myField1 END
FROM test;

Fixes: #49388

Previously, CaseProcessor was pre-calculating (called process())
on all the building elements of a CASE/IIF expression, not only the
conditions involved but also the results (if true/if false), as well as
the final else result. In case one of those results had an erroneous
calculation (e.g.: division by zero) this was executed and resulted in
an Exception to be thrown, even if this result was not used because of
the condition guarding it. e.g.:

```
SELECT CASE myField1 = 0 THEN NULL ELSE myField2 / myField1 END
FROM test;
```

Fixes: elastic#49388
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/SQL)

@matriv
Copy link
Contributor Author

matriv commented Nov 25, 2019

@astefan @costin I tried to add a unit test for this, but since in this we have fields involved (not only constants) I couldn't come up with one. Any ideas are welcome.

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 two cosmetic comments and in terms of tests, I would like to see an IT one with more than one WHEN branch.

List<Object> objects = new ArrayList<>(processors.size());
for (Processor processor : processors) {
objects.add(processor.process(input));
for (int i = 0; i < processors.size() - 2; i += 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, here you basically take a condition and you evaluate it, and if it holds then you evaluate its results and break out of the loop. If it doesn't hold (evaluates to false), then move on to the next set of [condition, result] and repeat the process. Can you add a comment about this, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

for (Processor processor : processors) {
objects.add(processor.process(input));
for (int i = 0; i < processors.size() - 2; i += 2) {
if (processors.get(i).process(input)==Boolean.TRUE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

processors.get(i).process(input) == Boolean.TRUE

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 dbd169a into elastic:master Nov 26, 2019
@matriv matriv deleted the fix-49388 branch November 26, 2019 09:47
matriv added a commit that referenced this pull request Nov 26, 2019
Previously, CaseProcessor was pre-calculating (called `process()`)
on all the building elements of a CASE/IIF expression, not only the
conditions involved but also the results, as well as the final else result.
In case one of those results had an erroneous calculation
(e.g.: division by zero) this was executed and resulted in
an Exception to be thrown, even if this result was not used because of
the condition guarding it. e.g.:

```
SELECT CASE myField1 = 0 THEN NULL ELSE myField2 / myField1 END
FROM test;
```

Fixes: #49388
(cherry picked from commit dbd169a)
matriv added a commit that referenced this pull request Nov 26, 2019
Previously, CaseProcessor was pre-calculating (called `process()`)
on all the building elements of a CASE/IIF expression, not only the
conditions involved but also the results, as well as the final else result.
In case one of those results had an erroneous calculation
(e.g.: division by zero) this was executed and resulted in
an Exception to be thrown, even if this result was not used because of
the condition guarding it. e.g.:

```
SELECT CASE myField1 = 0 THEN NULL ELSE myField2 / myField1 END
FROM test;
```

Fixes: #49388
(cherry picked from commit dbd169a)
matriv added a commit that referenced this pull request Nov 26, 2019
Previously, CaseProcessor was pre-calculating (called `process()`)
on all the building elements of a CASE/IIF expression, not only the
conditions involved but also the results, as well as the final else result.
In case one of those results had an erroneous calculation
(e.g.: division by zero) this was executed and resulted in
an Exception to be thrown, even if this result was not used because of
the condition guarding it. e.g.:

```
SELECT CASE myField1 = 0 THEN NULL ELSE myField2 / myField1 END
FROM test;
```

Fixes: #49388
(cherry picked from commit dbd169a)
@jpountz jpountz changed the title SQL: Fix issue with CASE/IIF pre-calculating results Fix issue with CASE/IIF pre-calculating results Dec 18, 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.

SQL: division by zero even if conditional function should have protected from this
5 participants