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: wildcard field type support #58062

Merged
merged 10 commits into from
Aug 17, 2020
Merged

QL: wildcard field type support #58062

merged 10 commits into from
Aug 17, 2020

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Jun 12, 2020

This PR adds support for wildcard data type following its addition in Elasticsearch with #49993.

There is one issue that will need to be addressed afterwards: there is a slightly different behavior between wildcard and the other text based data types (ie keyword) when it comes to Painless scripting. As soon as #58044 is fixed, the workaround in InternalQlScriptUtils needs to be reverted.

Addresses #54184.

@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jun 12, 2020
matriv
matriv reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Left some minor comments.
Also:

  • Can we have some Jdbc (PreparedStatement set() and ResultSet get()) tests.
  • Is it safe to merge it for 7.9 without the JDBC compatibility tests in place?

...a/org/elasticsearch/xpack/ql/expression/function/scalar/whitelist/InternalQlScriptUtils.java Outdated
@@ -32,7 +33,9 @@
if (doc.containsKey(fieldName)) {
ScriptDocValues<T> docValues = doc.get(fieldName);
if (!docValues.isEmpty()) {
return docValues.get(0);
T value = docValues.get(0);
// FIXME temporary workaround until https://github.com/elastic/elasticsearch/issues/58044 gets fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

The issues seems fixed, can you remove the workaround?

x-pack/plugin/sql/qa/server/src/main/resources/wildcard.csv-spec Outdated
;


singlePercentile
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't groupBy be more appropriate?

x-pack/plugin/sql/qa/server/src/main/resources/wildcard.csv-spec Outdated
Bezalel Simmel |10002.0
;

complexPercentile
Copy link
Contributor

Choose a reason for hiding this comment

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

and here complexGroupBy ?

costin
costin approved these changes Jun 16, 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. I too am concern about the lack of backwards compatibility...

bpintea
bpintea reviewed Jun 16, 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, except for the compatibility bit.
Generally looking at it, the new type doesn't seem to have an echo in the SQL functionality itself (much like constant-keyword vs keyword): are the three types distinguishable at SQL level, characteristics- and behavior- or restrictions-wise? B/c if they aren't, while strictly conceptually different types should indeed be reported, I'm wondering if we could compromise to ease the backwards compatibility restrains.

@astefan
Copy link
Contributor Author

astefan commented Jun 17, 2020

Currently blocked by: #56722
We need the bwc tests in place before adding a new data type.

@costin
Copy link
Member

costin commented Jun 18, 2020

B/c if they aren't, while strictly conceptually different types should indeed be reported, I'm wondering if we could compromise to ease the backwards compatibility restrains.

There is ongoing work on this front.

@astefan
Copy link
Contributor Author

astefan commented Jun 18, 2020

When the work in #53175 is done, this PR is not needed anymore. Also, CONSTANT_KEYWORD data type will also have to be removed from QL.

@costin
Copy link
Member

costin commented Jun 18, 2020

See #58315 as well.

@astefan astefan requested a review from costin August 3, 2020 12:17
@astefan astefan requested review from matriv and bpintea August 3, 2020 12:17
@costin
Copy link
Member

costin commented Aug 17, 2020

What's the status of this PR considering #58315 and #53175 have been closed?

@astefan
Copy link
Contributor Author

astefan commented Aug 17, 2020

@costin this PR has been updated after those PRs have been merged and now it contains only test code with the wildcard field. A wildcard field is regarded as a normal keyword field.

costin
costin reviewed Aug 17, 2020
View reviewed changes
x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/Types.java
@@ -48,8 +48,12 @@

private static DataType getType(DataTypeRegistry typeRegistry, Map<String, Object> content) {
if (content.containsKey("type")) {
String typeName = content.get("type").toString();
if ("wildcard".equals(typeName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Aren't wildcard returned as keywords? Or is this using a different API/source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's for testing. We use .json files having an index' complete mapping (see org.elasticsearch.xpack.ql.type.TypesTests) and the wildcard comparison is for bypassing SQL's and EQL's specific data types auto-resolver in the specific case of wildcard.

costin
costin approved these changes Aug 17, 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

@astefan astefan merged commit c874e6c into elastic:master Aug 17, 2020
@astefan astefan deleted the 54184_impl branch August 17, 2020 13:00
astefan added a commit to astefan/elasticsearch that referenced this pull request Aug 17, 2020
@astefan
QL: wildcard field type support (elastic#58062)
1fb45f2 
(cherry picked from commit c874e6c)
astefan added a commit to astefan/elasticsearch that referenced this pull request Aug 17, 2020
@astefan
QL: wildcard field type support (elastic#58062)
1c491ae 
(cherry picked from commit c874e6c)
astefan added a commit that referenced this pull request Aug 17, 2020
@astefan
QL: wildcard field type support (#58062) (#61205)
db8788e 
(cherry picked from commit c874e6c)
astefan added a commit that referenced this pull request Aug 17, 2020
@astefan
QL: wildcard field type support (#58062) (#61207)
bd37797 
(cherry picked from commit c874e6c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin costin costin approved these changes

@matriv matriv Awaiting requested review from matriv

@bpintea bpintea Awaiting requested review from bpintea

Assignees
No one assigned
Labels
:Analytics/EQL EQL querying :Analytics/SQL SQL querying >feature Team:QL (Deprecated) Meta label for query languages team v7.9.1 v7.10.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@astefan @elasticmachine @costin @bpintea @matriv @jakelandis @pugnascotia