Elements additions #62

Panos512 · 2016-10-04T11:26:19Z

Adds MalformedQuery element which is handled on various installations.
Adds Wildcard element to support queries containing wildcard characters (* and #).

* Adds a new ast element called MalformedQuery, this will be used by invenio_search to interpret malformed queries into google-like searches. Signed-off-by: Panos Paparrigopoulos <panos.paparrigopoulos@cern.ch>

jirikuncar

This is breaking syntax change. We need approval from more services. cc @egabancho @tiborsimko

jirikuncar · 2016-10-04T11:39:57Z

invenio_query_parser/ast.py

@@ -1,7 +1,8 @@
 # -*- coding: utf-8 -*-
 #
 # This file is part of Invenio-Query-Parser.
-# Copyright (C) 2014, 2016 CERN.
+#


jirikuncar · 2016-10-04T11:44:56Z

We should also check that Elastic query builder works with the new ast element.

jirikuncar · 2016-10-13T09:10:40Z

@Panos512 Please test new element with all walkers and contrib/elasticsearch. Thanks

egabancho · 2016-10-13T10:33:21Z

In principle I don't see any issue to include this, just a few questions

which difference does it make? I meant, with the current implementation the query it generates seems the same ...
why also # as wildcard and not only *?
what about *foo?

and as @jirikuncar mentioned

Please test new element with all walkers and contrib/elasticsearch

jacquerie · 2016-10-13T10:46:05Z

why also # as wildcard and not only *?

Legacy from SPIRES. The two symbols were equivalent there, and they are still equivalent in the current INSPIRE: https://inspirehep.net/search?ln=en&ln=en&p=find+j+%22Phys.Rev.Lett.%2C105*%22 and https://inspirehep.net/search?ln=en&ln=en&p=find+j+%22Phys.Rev.Lett.%2C105%23%22

Panos512 · 2016-10-13T11:27:16Z

which difference does it make? I meant, with the current implementation the query it generates > seems the same ...

For our case we treat WildcardQuery differently (https://github.com/inspirehep/inspire-next/blob/master/inspirehep/modules/search/walkers/elasticsearch.py#L94), and we are going to use the MalformedQuery element to show messages to users when they perform malformed queries.

what about *foo?

The problem with accepting wildcards everywhere in a word is that there are some particles that contain wildcard characters (e.g. D/sJ*(2632)). So accepting them only in the end of a word is partly solving the problem.

and as @jirikuncar mentioned

Please test new element with all walkers and contrib/elasticsearch

Apart from invenio-query-parser tests, that pass at the moment, do we test elasticsearch query generation somewhere else?

egabancho · 2016-10-13T13:37:07Z

The problem with accepting wildcards everywhere in a word is that there are some particles that contain wildcard characters (e.g. D/sJ*(2632)). So accepting them only in the end of a word is partly solving the problem.

IMHO this type of strings should be escaped using quotes to avoid ambiguity, i.e. "D/sJ*(2632)".
We have more cases like this, for example titles containing and or or, and not because of that we stopped using this keywords in the middle of queries.

Apart from invenio-query-parser tests, that pass at the moment, do we test elasticsearch query generation somewhere else?

Test are passing indeed but, you didn't add any test to the walkers to check that your change is not breaking them, which is what we are asking for.
In fact you didn't modify the walkers at all, that's why I wondered before which difference it makes 😉

Maybe if this is so inspire/spires centric you should consider updating contrib/spires instead (just a thought).

Panos512 · 2016-10-17T09:31:07Z

IMHO this type of strings should be escaped using quotes to avoid ambiguity, i.e. "D/sJ*(2632)".
We have more cases like this, for example titles containing and or or, and not because of that we stopped using this keywords in the middle of queries.

I'll take a look into this. Indeed it could be done like this and it's the optimal solution for us. But are we sure that the users are going to escape words with special characters? Maybe we could white-list all known particles and ignore special characters in them ( I could resurrect #39 for this ).

Test are passing indeed but, you didn't add any test to the walkers to check that your change is not breaking them, which is what we are asking for.
In fact you didn't modify the walkers at all, that's why I wondered before which difference it makes 😉

That's true, I will update the walkers + tests.

Maybe if this is so inspire/spires centric you should consider updating contrib/spires instead (just a thought).

The thing is that we need this functionality in the invenio-syntax and not only on spires-syntax.

* Adds wildcards (`*`, `#`) support to queries. Signed-off-by: Panos Paparrigopoulos <panos.paparrigopoulos@cern.ch>

kaplun · 2016-11-15T12:13:20Z

@jirikuncar I think @Panos512 has fullfilled the requests, and thus this PR is in good state to be merged.

jirikuncar · 2016-11-15T13:57:25Z

invenio_query_parser/ast.py

@@ -182,3 +186,7 @@ class RegexValue(Leaf):

 class EmptyQuery(Leaf):
    pass
+
+
+class MalformedQuery(Leaf):


do you still need MalformedQuery?

Ideally yes. This is then needed to implement: inspirehep/inspire-next#1182

In that case I don't see any test for it.

jirikuncar · 2016-11-15T13:58:54Z

invenio_query_parser/contrib/elasticsearch/walkers/dsl.py

@@ -80,6 +81,24 @@ def visit(self, node, op):
    def visit(self, node):
        return node.value

+    @visitor(WildcardQuery)
+    def visit(self, node):
+        def query(keyword):


I don't see tests for this part.

Panos has now left. Can you maybe guide us on concretely what type of tests we would need to write?

see https://github.com/inveniosoftware/invenio-query-parser/blob/master/tests/test_walkers.py#L80

jirikuncar · 2016-11-15T13:59:07Z

invenio_query_parser/contrib/elasticsearch/walkers/dsl.py

+        def query(keyword):
+            fields = self.get_fields_for_keyword(keyword, mode='p')
+            if len(fields) > 1:
+                res = Q('bool', should=[


return Q('bool', should=[

jirikuncar · 2016-11-15T13:59:28Z

invenio_query_parser/contrib/elasticsearch/walkers/dsl.py

+                          query=value,
+                          default_field=k,
+                          analyze_wildcard=True) for k in fields])
+            else:


- else + return Q('query_string',

lnielsen · 2022-02-16T21:11:20Z

Closing PR as it's been stale for a long time.

parser: new ast element

40122f2

* Adds a new ast element called MalformedQuery, this will be used by invenio_search to interpret malformed queries into google-like searches. Signed-off-by: Panos Paparrigopoulos <panos.paparrigopoulos@cern.ch>

jirikuncar added the Status: in review label Oct 4, 2016

Panos512 changed the title ~~Parsing additions~~ Elements additions Oct 4, 2016

Panos512 mentioned this pull request Oct 4, 2016

WIP global: adds invenio_query_parser dependency inspirehep/inspire-next#1624

Closed

jirikuncar requested changes Oct 4, 2016

View reviewed changes

jirikuncar added this to the v1.0.0 milestone Oct 4, 2016

Panos512 force-pushed the invenio_original branch from 38e4f68 to d8bf50c Compare October 4, 2016 11:47

jirikuncar assigned tiborsimko and egabancho Oct 13, 2016

jirikuncar added the Need: tests label Oct 13, 2016

jirikuncar assigned Panos512 Oct 13, 2016

Panos512 force-pushed the invenio_original branch from d8bf50c to 01f50d6 Compare October 18, 2016 08:59

global: enables wildcards

2a16737

* Adds wildcards (`*`, `#`) support to queries. Signed-off-by: Panos Paparrigopoulos <panos.paparrigopoulos@cern.ch>

Panos512 force-pushed the invenio_original branch from 01f50d6 to 2a16737 Compare October 26, 2016 11:12

jirikuncar reviewed Nov 15, 2016

View reviewed changes

jacquerie self-assigned this Nov 15, 2016

lnielsen closed this Feb 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elements additions #62

Elements additions #62

Panos512 commented Oct 4, 2016

jirikuncar left a comment

jirikuncar Oct 4, 2016

jirikuncar commented Oct 4, 2016

jirikuncar commented Oct 13, 2016

egabancho commented Oct 13, 2016

jacquerie commented Oct 13, 2016 •

edited

Panos512 commented Oct 13, 2016

egabancho commented Oct 13, 2016

Panos512 commented Oct 17, 2016

kaplun commented Nov 15, 2016

jirikuncar Nov 15, 2016

kaplun Nov 15, 2016

jirikuncar Nov 15, 2016

jirikuncar Nov 15, 2016

kaplun Nov 15, 2016

jirikuncar Nov 15, 2016

jirikuncar Nov 15, 2016

jirikuncar Nov 15, 2016

lnielsen commented Feb 16, 2022

Elements additions #62

Elements additions #62

Conversation

Panos512 commented Oct 4, 2016

jirikuncar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirikuncar commented Oct 4, 2016

jirikuncar commented Oct 13, 2016

egabancho commented Oct 13, 2016

jacquerie commented Oct 13, 2016 • edited

Panos512 commented Oct 13, 2016

egabancho commented Oct 13, 2016

Panos512 commented Oct 17, 2016

kaplun commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnielsen commented Feb 16, 2022

jacquerie commented Oct 13, 2016 •

edited