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

QL: Remove implicit conversion inside Literal #50962

Merged
merged 4 commits into from
Jan 15, 2020

Conversation

costin
Copy link
Member

@costin costin commented Jan 14, 2020

Literal constructor makes an implicit conversion for each value given
which turns out has some subtle side-effects.
Improve MathProcessors to preserve numeric type where possible
Fix bug on issue compatibility between date and intervals
Preserve the source when folding inside the Optimizer

Literal constructor makes an implicit conversion for each value given
which turns out has some subtle side-effects.
Improve MathProcessors to preserve numeric type where possible
Fix bug on issue compatibility between date and intervals
Preserve the source when folding inside the Optimizer
@elasticmachine
Copy link
Collaborator

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

@costin
Copy link
Member Author

costin commented Jan 14, 2020

@elasticmachine run elasticsearch-ci/2

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

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. Left 2 comments regarding testing.

Moreover, shouldn't we backport the fix regarding the DATE +/- time INTERVAL?


if (l instanceof Integer) {
if (lo == Integer.MIN_VALUE) {
throw new QlIllegalArgumentException("[" + lo + "] cannot be negated since the result is outside the range");
Copy link
Contributor

Choose a reason for hiding this comment

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

This also?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added basic test

@costin
Copy link
Member Author

costin commented Jan 14, 2020

Moreover, shouldn't we backport the fix regarding the DATE +/- time INTERVAL?

It will once the 7.x freeze passes and the QL extraction stabilizes.

@costin costin merged commit 9b73e22 into elastic:master Jan 15, 2020
@costin costin deleted the ql/datatype-refactor branch January 15, 2020 09:06
@matriv
Copy link
Contributor

matriv commented Jan 15, 2020

Moreover, shouldn't we backport the fix regarding the DATE +/- time INTERVAL?

It will once the 7.x freeze passes and the QL extraction stabilizes.

I ment to previous versions, 7.5 & 6.8

SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
Literal constructor makes an implicit conversion for each value given
which turns out has some subtle side-effects.
Improve MathProcessors to preserve numeric type where possible
Fix bug on issue compatibility between date and intervals
Preserve the source when folding inside the Optimizer
costin added a commit that referenced this pull request Jan 27, 2020
* Introduce reusable QL plugin for SQL and EQL (#50815)

Extract reusable functionality from SQL into its own dedicated project QL.
Implemented as a plugin, it provides common components across SQL and the upcoming EQL.

While this commit is fairly large, for the most part it's just a big file move from sql package to the newly introduced ql.

(cherry picked from commit ec1ac0d)

* SQL: Fix incomplete registration of geo NamedWritables

(cherry picked from commit e295763)

* QL: Extend NodeSubclass to read classes from jars (#50866)

As the test classes are spread across more than one project, the Gradle
classpath contains not just folders but also jars.
This commit allows the test class to explore the archive content and
load matching classes from said source.

(cherry picked from commit 25ad749)

* QL: Remove implicit conversion inside Literal (#50962)

Literal constructor makes an implicit conversion for each value given
which turns out has some subtle side-effects.
Improve MathProcessors to preserve numeric type where possible
Fix bug on issue compatibility between date and intervals
Preserve the source when folding inside the Optimizer

(cherry picked from commit 9b73e22)

* QL: Refactor DataType for pluggability (#51328)

Change DataType from enum to class
Break DataType enums into QL (default) and SQL types
Make data type conversion pluggable so that new types can be introduced

As part of the process:
- static type conversion in QL package (such as Literal) has been
removed
- several utility classes have been broken into base (QL) and extended
(SQL) parts based on type awareness
- operators (+,-,/,*) are
- due to extensibility, serialization of arithmetic operation has been
slightly changed and pushed down to the operator executor itself

(cherry picked from commit aebda81)

* Compilation fixes for 7.x
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.

None yet

6 participants