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

QL: Remove constant_keyword field type #60524

Merged
merged 9 commits into from
Nov 5, 2020
Merged

QL: Remove constant_keyword field type #60524

merged 9 commits into from
Nov 5, 2020

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Jul 31, 2020

Removes constant_keyword field type from *QL, after the keyword family of fields has been added to Elasticsearch.

Addresses #58471.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/SQL)

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jul 31, 2020
@astefan
Copy link
Contributor Author

astefan commented Aug 3, 2020

@elasticmachine run elasticsearch-ci/packaging-sample-windows

bpintea
bpintea approved these changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

LGTM and the tests I've run work as expected.
However, there is a theoretical possibility that an old driver (say 7.8) will send a request with a parameter of this type (to a 7.10 server). With the PR we'd reject the request which does break the backwards compatibility strictly speaking. But I believe this is such a small corner case and so unlikely that it's negligible.

@Mpdreamz Mpdreamz added v7.9.2 and removed v7.9.1 labels Sep 1, 2020
@danhermann danhermann added v7.9.3 and removed v7.9.2 labels Sep 18, 2020
@bpintea
Copy link
Contributor

bpintea commented Oct 16, 2020

there is a theoretical possibility that an old driver (say 7.8) will send a request with a parameter of this type (to a 7.10 server). With the PR we'd reject the request which does break the backwards compatibility strictly speaking.

Looking deeper into the issue, the statement above is wrong and I think we could still safely merge the PR (past the PR conflict resolution).
As far as the JDBC client is concerned, the types we report are uniquely identified by their name, not DATE_TYPE or some other ID:

        TYPE_NAME        |   DATA_TYPE   |   ...
-------------------------+---------------+---------------+-...
CONSTANT_KEYWORD         |12             |...
IP                       |12             |...
KEYWORD                  |12             |...
TEXT                     |12             |...

We don't export a "ConstantKeyword" class in our JDBC driver and the the JDBC interface won't take a type name when setting parameters, but only a SQLType, which maps the reported DATA_TYPE and which is always a VARCHAR (so changing that enum is luckly irrelevant). Meaning that all our JDBC driver releases send KEYWORD parameter types.

The only way to send a CONSTANT_KEYWORD parameter is directly over the REST if'ace (which btw, I'm not sure if it's bwc-tested?). And which might still not be worth the effort of accommodating for, backwards compatibility-wise.

WDYT, @costin, @astefan?

@matriv matriv added v7.9.4 and removed v7.9.3 labels Oct 16, 2020
@astefan astefan mentioned this pull request Oct 19, 2020
Closed
@costin
Copy link
Member

costin commented Oct 26, 2020

LGTM

@costin costin self-requested a review October 26, 2020 14:25
costin
costin reviewed Oct 26, 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

@bpintea bpintea mentioned this pull request Oct 26, 2020
Closed
@bpintea
Correct conflict merge resolution
a5bf455 
Both "wildcard" and "constant_keyword" need to return "keyword" as
mapped type.
@bpintea
Copy link
Contributor

bpintea commented Oct 30, 2020

@elasticmachine update branch

@bpintea
Copy link
Contributor

bpintea commented Oct 30, 2020

@elasticmachine run elasticsearch-ci/packaging-sample-unix

@bpintea bpintea added v7.10.1 and removed v7.9.4 labels Nov 5, 2020
@bpintea
Copy link
Contributor

bpintea commented Nov 5, 2020

@elasticmachine update branch

@bpintea bpintea merged commit d9ba96c into elastic:master Nov 5, 2020
bpintea pushed a commit to bpintea/elasticsearch that referenced this pull request Nov 5, 2020
@astefan @bpintea
QL: Remove constant_keyword field type (elastic#60524)
908b754 
* Remove constant_keyword field type

(cherry picked from commit d9ba96c)
@bpintea bpintea mentioned this pull request Nov 5, 2020
Merged
bpintea added a commit that referenced this pull request Nov 5, 2020
@bpintea @astefan
QL: Remove constant_keyword field type (#60524) (#64661)
1d54310 
* Remove constant_keyword field type

(cherry picked from commit d9ba96c)

Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com>
bpintea added a commit to bpintea/elasticsearch that referenced this pull request Nov 6, 2020
@bpintea
Remove constant_keyword from SQL docs
0eb8bde 
`constant_keyword` removed as distinct type from SQL in elastic#60524.
@bpintea bpintea mentioned this pull request Nov 6, 2020
Merged
bpintea added a commit that referenced this pull request Nov 6, 2020
@bpintea @jrodewig
SQL: Remove constant_keyword from SQL docs (#64709)
6a92b3e 
* Remove constant_keyword from SQL docs

`constant_keyword` removed as distinct type from SQL in #60524.

Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
bpintea added a commit that referenced this pull request Nov 6, 2020
@bpintea
SQL: Remove constant_keyword from SQL docs (#64709)
79446e2 
* Remove constant_keyword from SQL docs

`constant_keyword` removed as distinct type from SQL in #60524.

Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
(cherry picked from commit 6a92b3e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpintea bpintea bpintea approved these changes

@costin costin costin approved these changes

Assignees
No one assigned
Labels
:Analytics/EQL EQL querying :Analytics/SQL SQL querying >refactoring 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.
8 participants
@astefan @elasticmachine @bpintea @costin @Mpdreamz @jakelandis @matriv @danhermann