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

EQL: Remove (was Deprecate) = in favor of == for equality checks #62650

Closed
costin opened this issue Sep 18, 2020 · 8 comments · Fixed by #62756
Closed

EQL: Remove (was Deprecate) = in favor of == for equality checks #62650

costin opened this issue Sep 18, 2020 · 8 comments · Fixed by #62756
Assignees
Labels
:Analytics/EQL EQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team

Comments

@costin
Copy link
Member

costin commented Sep 18, 2020

A topic that was raised in #61883 (comment) is deprecating usage of = when doing equality checks.
EQL does use = for assignments so using it for equality checks instead of == is potentially confusing (is it an assignment or an equality check?).
It would be good to get some numbers on the usage to see how often this occurs.
Based on that we can decide whether to go with a safe route (deprecation-only with removal later down the road) or straight removal.

@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 Sep 18, 2020
@jrodewig
Copy link
Contributor

jrodewig commented Sep 18, 2020

As a note for consideration: Neither the Python nor the ES implementation document = as an equality operator.

Python: https://eql.readthedocs.io/en/latest/query-guide/basic-syntax.html#conditions
ES: https://www.elastic.co/guide/en/elasticsearch/reference/master/eql-syntax.html#eql-syntax-conditions

Unless usage is high, we could potentially treat this as a bug and just remove it.

@rw-access
Copy link
Contributor

rw-access commented Sep 18, 2020

Agreed. Usage of = for equality comparisons was in a gray area. It was supported out of convenience and flexibility, but never documented. This was a bit of a safety net.

We should continue to leave a single = for the maxspan= and also underdocumented fork= syntax. Similarly, I don't think there are any examples of maxspan==1m.

And of course, in both cases, I think we should continue to accept both forms in the grammar, but take the usual approach of raising a custom error message to say it's no longer supported.

Here are the usage numbers rules in our Endgame rules repo, but we tend to conform to the same convention and have PR review, so ours will be biased towards ==. In the wild, I think the usage of = is more common than this, but if we have a helpful error message, we can at least nudge users on to the right path.

  • 19 =
  • 1201 ==

@astefan
Copy link
Contributor

astefan commented Sep 21, 2020

I agree with completely removing the single equals (=) option, even more if this was never documented and was supported just as a convenience alternative. Keeping both around leaves room for ambiguities and is not necessary (equality should refer to a single operator). I do also want to remove it without any deprecation or error messages being logged since:

  • in ES ecosystem if a feature is not documented then it is not officially supported, thus it can change at any time without any warning or deprecation message
  • we should have a beta/GA version as "clean" as possible, without ambiguities in the language spec since these tend to change very rarely to not at all afterwards. Also, our users should have a clear picture of how the language looks like before they start using EQL in production

@matriv
Copy link
Contributor

matriv commented Sep 21, 2020

I also prefer to completely remove the = and have a clear grammar before beta->GA.

@costin
Copy link
Member Author

costin commented Sep 22, 2020

@sethpayne asked

Given that we'll be supporting runtime fields in ES soon, should we reserve = for defining these fields in EQL?

What do you have in mind. = is still being used for maxspan declaration maxspan=1m and thus whenever we have assignments. I'd like for runtime fields to be as transparent as possible.

@costin
Copy link
Member Author

costin commented Sep 22, 2020

Thanks for the feedback folks.

Considering it's an undocumented behavior (and thus without any guarantees) used in 1.5% of the cases, let's move ahead with removing = from bool expressions without going through deprecation.

@matriv can you please look at it?

Thanks

@costin costin changed the title EQL: Deprecate = in favor of == for equality checks EQL: ~~Deprecate~~ Remove = in favor of == for equality checks Sep 22, 2020
@costin costin changed the title EQL: ~~Deprecate~~ Remove = in favor of == for equality checks EQL: Remove (was Deprecate) = in favor of == for equality checks Sep 22, 2020
@matriv matriv self-assigned this Sep 22, 2020
matriv added a commit to matriv/elasticsearch that referenced this issue Sep 22, 2020
Since `=` is rarely used and is undocumented we its support for
equality comparisons keeping `==` as the only option. `=` is now only
used for assignements like in `maxspan=10m`.

Closes: elastic#62650
matriv added a commit that referenced this issue Sep 22, 2020
Since `=` is rarely used and is undocumented we its support for
equality comparisons keeping `==` as the only option. `=` is now only
used for assignments like in `maxspan=10m`.

Closes: #62650
matriv added a commit that referenced this issue Sep 22, 2020
Since `=` is rarely used and is undocumented we its support for
equality comparisons keeping `==` as the only option. `=` is now only
used for assignments like in `maxspan=10m`.

Closes: #62650
(cherry picked from commit ad5ae4d)
@matriv
Copy link
Contributor

matriv commented Sep 22, 2020

master : ad5ae4d
7.x : 1e72144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/EQL EQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants