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: rewrite ROUND and TRUNCATE functions with a different optional parameter handling method #40242

Merged
merged 8 commits into from Mar 21, 2019

Conversation

Projects
None yet
6 participants
@astefan
Copy link
Contributor

commented Mar 20, 2019

The optional parameter for these two functions was handled in the constructor by passing the value of 0 whenever the parameter was missing. This had an apparently minor implication of defining a second child for the function (the second parameter). If the value was kept as null (missing) the function would have had a single child (the first parameter).

This affected the comparison operation from the Analyzer and the same function that was already resolved wouldn't match the new function found because the new one had no second parameter while the resolved one had the parameter 0. Because of this comparison failing, a GROUP BY the same function would fail.

This PR also fixes a bug with ATAN2 and POWER functions which didn't have any Painless methods.

Fixes #40001.

astefan added some commits Mar 20, 2019

Rewrite Round and Truncate functions to have a slightly different
approach to handling the optional parameter in the constructor. Until now
the optional parameter was considered 0 if the value was missing and the
constructor was filling in this value. The current solution is to have
the optional parameter as null right until the actual calculation is done.
@elasticmachine

This comment has been minimized.

Copy link

commented Mar 20, 2019

@matriv
Copy link
Contributor

left a comment

Nice refactoring. Left some comments.


}

return right == null ? TypeResolution.TYPE_RESOLVED : isNumeric(right, sourceText(), ParamOrdinal.SECOND);

This comment has been minimized.

Copy link
@matriv

matriv Mar 20, 2019

Contributor

The right should be of integer type (not rational), so we should catch it here before the execution time.

public DataType dataType() {
return left().dataType();
public Expression replaceChildren(List<Expression> newChildren) {
if (right() != null && newChildren.size() != 2) {

This comment has been minimized.

Copy link
@matriv

matriv Mar 20, 2019

Contributor

I'd extract this to the parent method and leave here only the instance creation, as it's duplicated also in Round.

}

private ScriptTemplate asScriptFrom(ScriptTemplate leftScript, ScriptTemplate rightScript) {
if (right == null) {

This comment has been minimized.

Copy link
@matriv

matriv Mar 20, 2019

Contributor

Wouldn't be possible to avoid the "double" methods in painless and just have one with both args where null is passed for the right arg?

This comment has been minimized.

Copy link
@astefan

astefan Mar 20, 2019

Author Contributor

Gave it a try, but failed. If you have any ideas, I'd love to hear them. Otherwise, I'll try later today or tomorrow.

This comment has been minimized.

Copy link
@matriv

matriv Mar 20, 2019

Contributor

I took some time here and it works like that: matriv@b35090e

This comment has been minimized.

Copy link
@astefan

astefan Mar 21, 2019

Author Contributor

Indeed. Nice one. I've incorporated it in the PR. Thanks.

import java.util.function.BiFunction;

/**
* Processor for binary mathematical operations that can have the second parameter optional.

This comment has been minimized.

Copy link
@matriv

matriv Mar 20, 2019

Contributor

Maybe rephrase: ... that have a second optional parameter.

@@ -200,6 +200,23 @@ ROUND(SQRT(CAST(EXP(languages) AS SMALLINT)),2):d| COUNT(*):l
null |10
;

groupByRoundWithTwoParams

This comment has been minimized.

Copy link
@matriv

matriv Mar 20, 2019

Contributor

Can't this be tested in QueryTranslatorTests?

astefan added some commits Mar 20, 2019

@astefan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/1

@astefan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/default-distro

3 similar comments
@astefan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/default-distro

@astefan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@elasticmachine run elasticsearch-ci/default-distro

@astefan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@elasticmachine run elasticsearch-ci/default-distro

@matriv

matriv approved these changes Mar 21, 2019

Copy link
Contributor

left a comment

LGTM

@danielmitterdorfer danielmitterdorfer added v6.7.1 and removed v6.7.0 labels Mar 21, 2019

@costin

costin approved these changes Mar 21, 2019

Copy link
Member

left a comment

Considering the timing and everything, I'm fine with the approach.
Going forward I think we need a more generic way of dealing with optional arguments - whether that requires updates in the Analyzer or/and the functions remains to be determined.

@astefan astefan merged commit 3e314f8 into elastic:master Mar 21, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@astefan astefan deleted the astefan:40001_fix branch Mar 21, 2019

astefan added a commit that referenced this pull request Mar 22, 2019

SQL: rewrite ROUND and TRUNCATE functions with a different optional p…
…arameter handling method (#40242)

* Rewrite Round and Truncate functions to have a slightly different
approach to handling the optional parameter in the constructor. Until now
the optional parameter was considered 0 if the value was missing and the
constructor was filling in this value. The current solution is to have
the optional parameter as null right until the actual calculation is done.

(cherry picked from commit 3e314f8)

astefan added a commit that referenced this pull request Mar 22, 2019

SQL: rewrite ROUND and TRUNCATE functions with a different optional p…
…arameter handling method (#40242)

* Rewrite Round and Truncate functions to have a slightly different
approach to handling the optional parameter in the constructor. Until now
the optional parameter was considered 0 if the value was missing and the
constructor was filling in this value. The current solution is to have
the optional parameter as null right until the actual calculation is done.

(cherry picked from commit 3e314f8)

astefan added a commit that referenced this pull request Mar 22, 2019

SQL: rewrite ROUND and TRUNCATE functions with a different optional p…
…arameter handling method (#40242)

* Rewrite Round and Truncate functions to have a slightly different
approach to handling the optional parameter in the constructor. Until now
the optional parameter was considered 0 if the value was missing and the
constructor was filling in this value. The current solution is to have
the optional parameter as null right until the actual calculation is done.

(cherry picked from commit 3e314f8)

astefan added a commit that referenced this pull request Mar 22, 2019

SQL: rewrite ROUND and TRUNCATE functions with a different optional p…
…arameter handling method (#40242)

* Rewrite Round and Truncate functions to have a slightly different
approach to handling the optional parameter in the constructor. Until now
the optional parameter was considered 0 if the value was missing and the
constructor was filling in this value. The current solution is to have
the optional parameter as null right until the actual calculation is done.

(cherry picked from commit 3e314f8)

@michaelbaamonde michaelbaamonde added v7.0.0-rc1 and removed v7.0.0 labels Mar 25, 2019

pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Mar 25, 2019

SQL: rewrite ROUND and TRUNCATE functions with a different optional p…
…arameter handling method (elastic#40242)

* Rewrite Round and Truncate functions to have a slightly different
approach to handling the optional parameter in the constructor. Until now
the optional parameter was considered 0 if the value was missing and the
constructor was filling in this value. The current solution is to have
the optional parameter as null right until the actual calculation is done.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.