Skip to content

Add EXISTS condition to query grammar#4077

Merged
melekes merged 13 commits intotendermint:masterfrom
haasted:add-occurs
Nov 7, 2019
Merged

Add EXISTS condition to query grammar#4077
melekes merged 13 commits intotendermint:masterfrom
haasted:add-occurs

Conversation

@haasted
Copy link
Contributor

@haasted haasted commented Oct 24, 2019

This PR adds an "EXISTS" condition to the event query grammar. It enables querying for the occurrence of an event without having to provide a condition for one of its attributes.

As an example, someone interested in all slashing events might currently catch them with a query such as slash.power > 0.

With this PR the event can be captured with slash.power EXISTS or just slash EXISTS to catch by event type.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

for c := 0; c < int(token.next); c++ {
fmt.Printf(" ")
}
fmt.Printf("\x1B[34m%v\x1B[m %v\n", rul3s[token.pegRule], strconv.Quote(string(([]rune(buffer)[token.begin:token.end]))))
Copy link
Contributor

Choose a reason for hiding this comment

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

line is 123 characters (from lll)

@melekes
Copy link
Contributor

melekes commented Oct 24, 2019

Alternative syntax proposal: slash.power = * (refs: #1421)

@haasted
Copy link
Contributor Author

haasted commented Oct 25, 2019

@melekes :
Thanks for your feedback!

I like the idea of using JSON-query, although I see two arguments against it:

  1. The data being queried is not really JSON. It’s a fairly fixed key-value structure
  2. The rest of the query syntax doesn’t seem to adhere to jsquery.

@melekes
Copy link
Contributor

melekes commented Oct 25, 2019

I am not entirely sure js-query is a good idea, but extending current format without clear understanding how it will evolve is either bad.

@melekes
Copy link
Contributor

melekes commented Oct 25, 2019

key EXISTS looks fine to me though

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

Could you please add an entry to CHANGELOG_PENDING.md?

Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>

loop:
for k := range events {
if strings.Index(k, tag) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

why Index? Shouldn't we use strict comparison here ?

Suggested change
if strings.Index(k, tag) == 0 {
if k == tag {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case where the query is the "event type", e.g. just "slash". The contents of the events slice are the fully qualified event attributes, e.g. slash.reason. The index is used to verify that the event starts with the tag from the query.

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't it lead to false positives? I.e. sl EXISTS will work for slash.reason, but I doubt that it's what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sl EXISTS is valid syntax in the current implementation. Not sure whether I would consider it a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be reflected both in the docs

To tell which events you want, you need to provide a query. query is a
and in the tests. Thank you 🙇

@haasted haasted requested a review from melekes October 28, 2019 12:33
@melekes
Copy link
Contributor

melekes commented Nov 4, 2019

#4077 (comment) needs to be addressed before we can merge.

@haasted
Copy link
Contributor Author

haasted commented Nov 5, 2019

Addressed comments.

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

@melekes melekes merged commit 98c5953 into tendermint:master Nov 7, 2019
@melekes melekes mentioned this pull request Nov 11, 2019
5 tasks
@thith thith mentioned this pull request Apr 28, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants