Skip to content

Conversation

@plajjan
Copy link
Member

@plajjan plajjan commented Mar 27, 2015

This changes the query interpretation format to a new and improved format that is able to correctly model a query. The old format is a list with an entry for each element, which doesn't allow correctly grouping a more complex query.

For example, the query 'a b c' means we have three elements; a, b and c. There is an implicit boolean AND operator between them and so the query interpretation could be correctly represented with just a list and an implicit AND operator. However, as we wish to support a query like 'a AND (b or c)' we now have a different operator and it is (explicitly) grouped in a way that isn't possible to express with a flat list. The new format follows the query format, ie it is a hierarchy and the new query interpretation format can thus express all possible queries.

@plajjan
Copy link
Member Author

plajjan commented Mar 27, 2015

@garberg I'm not sure we should actually merge this just yet but I want your feedback on the look of it at least. I have updated the web UI to display the new format but it isn't very visually pleasing so before merging to master we might want to revisit that and perhaps write something with AngularJS.

@plajjan
Copy link
Member Author

plajjan commented Mar 29, 2015

I realize I should perhaps comment on the structure, because while the interpretation format can closely follow the query hierarchy it doesn't have to. In fact, in many cases it is desirable that it does not.

Performing a smart_search_pool on the string 'foo' will result in a query that looks like this:

'operator': 'or',
'val1': {
   'operator': 'regex',
   'val1': 'name',
   'val2': 'foo'
},
'val2': {
   'operator': 'regex',
   'val1': 'description',
   'val2': 'foo'
}

And inserting the query interpretation in this, it could look like this:

'operator': 'or',
'val1': {
    'interpretation': {
        'attribute': 'name',
        'interpretation': 'text',
        'operator': 'regex',
        'string': 'foo'
    },
    'operator': 'regex',
    'val1': 'name',
    'val2': 'foo'
},
'val2': {
    'interpretation': {
        'attribute': 'name',
        'interpretation': 'text',
        'operator': 'regex',
        'string': 'foo'
    },
    'operator': 'regex',
    'val1': 'description',
    'val2': 'foo'
}

Which in turn would generate two interpretation lines in the CLI or web UI.

The alternative is this:

'interpretation': {
    'attribute': 'name or description',
    'interpretation': 'text',
    'operator': 'regex',
    'string': 'foo'
},
'operator': 'or',
'val1': {
    'operator': 'regex',
    'val1': 'name',
    'val2': 'foo'
},
'val2': {
    'operator': 'regex',
    'val1': 'description',
    'val2': 'foo'
}

Ie, we let one interpretation node describe a deeper structure so that it a bit briefer. This is often the preferred way.

@garberg
Copy link
Member

garberg commented Apr 16, 2015

The only comment I have is regarding the different keys in the interpretation-dict which varies a bit in the smart_search_prefix-function. Depending on the input the interpretation sometimes has an expanded-key and/or a strict_prefix-key, but as this is not introduced in this pull request it's not really relevant here.

As soon as the interpretation is displayed prettier in the web UI I'm happy with merging this pull request.

@plajjan
Copy link
Member Author

plajjan commented May 2, 2015

What do you think of this format for the CLI:

kll@lingloi320 ~/kod/NIPAP/nipap-cli $ ./nipap address list --show-interpretation a b c d e
Searching for prefixes in any VRF...
Query interpretation:
      AND-- e: text matching e
        `-- AND-- d: text matching d
              `-- AND-- c: text matching c
                    `-- AND-- b: text matching b
                          `-- a: text matching a
No addresses matching 'a b c d e' found.

As compared to what the PR currently renders:

kll@lingloi320 ~/kod/NIPAP/nipap-cli $ ./nipap address list --show-interpretation a b c d e
Searching for prefixes in any VRF...
Query interpretation:
 - AND
   - e: text matching e
   - AND
     - d: text matching d
     - AND
       - c: text matching c
       - AND
         - b: text matching b
         - a: text matching a
No addresses matching 'a b c d e' found.

I quite like it. It's more compact (not width-wise though) and is easier to read.

@plajjan plajjan added this to the Version 0.28 - Hades milestone May 4, 2015
@plajjan
Copy link
Member Author

plajjan commented May 4, 2015

Okay, here's a new version of the interpretation in the web UI
nipap-interp-new
Compared to the "old" way (ie, not git master, but as currently suggested in this patch).
nipap-interp-old
I don't think there's any doubt on that the new format is way better. Does it need further improvement or is this enough?

I know alignment is a little bit off but this is a hack. I really don't want to spend more time on it right now. It should be rewritten as AngularJS thingy anyway.

@garberg feedback plxz. This is the last thing for this PR. Please comment on looks of interpretation in CLI (previous comment) and web UI. If we are happy, I'll update the PR and we should be good to merge.

@garberg
Copy link
Member

garberg commented May 4, 2015

Looks great! The hierarchies are quite deep, but that's just motivation to build the support for multi-parameter operators (so only one AND-operator is needed for multiple values) we've spoken about for so long time 😄

plajjan added 14 commits May 4, 2015 21:43
The code for parsing a smart search prefix query wasn't very testable as
it issued a database query after parsing the query. This has been
refactored into two functions so that the parsing is done separately and
thus becomes testable.

Four tests are added to check for three different types of queries and a
fourth to check that we correctly detect non-balanced quotes.
Instead of sending a separate data struct (which was just a list) with
the query interpretation, we modify the query dictSQL and insert our
interpretation into it which means that the change of discrepancy
between the actual query and the interpretation is minimized.

Naturally, this new format allows us to express a much more complex
query than before when we assumed all entries in the interpretation list
to be ANDed together.

This change only affects the smart_search_prefix function at this time
and subsequently the format for VRFs and pool searches remain the same.

Part of SpriteLink#683.
The CLI now accepts the improved query interpretation format implemented
by nipapd for prefixes.

Part of SpriteLink#683.
The web UI now accepts the improved prefix query interpretation format
implemented in nipapd.

We need to work on the layout of the interpretation as it is quite
verbose and not so pretty in its current incarnation. I suppose we
should rewrite the whole thing using AngularJS as well, unfortunately
I'm on a plane and haven't worked enough with Angular to remember from
the top of my head how to implement this nicely.

Part of SpriteLink#683.
The unittests testing the smart parser now passes as the expected
results are updated with the extra interpretation struct according to
the new and improved query interpretation format for prefixes.

Part of SpriteLink#683.
This implements for VRF the same improved query interpretation format
that was implemeneted for prefixes. In essence, it is more tightly
integrated with the query and is therefore just as expressive as the
query itself.

Part of SpriteLink#683.
The CLI now accepts the improved query interpretation format
implemented by the backend for VRFs.

Part of SpriteLink#683.
This implements for pools the same improved query interpretation format
that was implemented for prefixes and VRFs.

Part of SpriteLink#683.
The CLI now accepts the improved query interpretation format implemented
by the backend for pools.

Part of SpriteLink#683.
This implements for ASNs the same improved query interpretation format
that was implemented for prefixes, VRFs and pools.

Part of SpriteLink#683.
In addition to testing the prefix smart query parser this adds similar
tests for the smart query parsers of VRF and pool queries.

Part of SpriteLink#683.
Flip order of showing interpretation and checking if we got any results
so that we always display the interpretation, even when we don't find
any matches.

Part of SpriteLink#683.
This makes it much easier to overlook the interpretation. Visual
brackets are used to show which atoms a boolean operator (AND/OR) joins
together. It is also more compact than the previous format.

Part of SpriteLink#683.
Instead of printing AND operators on a separate line they are now on
the same line as a match statement making the output a lot more compact
in number of lines, although a bit wider. Through some ASCII art we try
to show which statements are joined together by a boolean operator.

I believe readability is greatly enhanced by this change, though that
might be biased ;)
@plajjan plajjan force-pushed the 683-improve-interpretation branch from 171d0a3 to 85d5a25 Compare May 4, 2015 21:37
@plajjan
Copy link
Member Author

plajjan commented May 4, 2015

Okay, did a slight rebase so had to force push.

@garberg This should now be ready to go. Have a last read if you want and push the magic button ;)

@garberg
Copy link
Member

garberg commented May 5, 2015

k'POW!

garberg added a commit that referenced this pull request May 5, 2015
@garberg garberg merged commit 9ae0af6 into SpriteLink:master May 5, 2015
@plajjan plajjan deleted the 683-improve-interpretation branch May 5, 2015 20:51
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.

2 participants