Add basic but flexible query language #308

yarikoptic · 2024-02-21T02:10:36Z

An original target fake query which has various features was

((jim AND NOT haxby AND "important\" paper") OR ds_id:~"^000[3-9]..$" OR url:"example.com") AND metadata:non AND metadata[ex1,ex2]:"specific data" AND metadata[extractor2]:data

and this PR implements most of them -- check the modal window description of supported syntax:

quite flexible implementation so we could add/replace fields we are searching on
can search within specified metadata extractors
- TODO (not here): search within JSON records is not part of this PR -- but likely would just allow to pass JSONPath for the most flexible functionality
- TODO (not here): expose via API call. Expose current search via API #269
has help on syntax supporting boolean operations etc. Closes improve search: use AND across search terms #264
replacing original /search

…eries Using lark we establish grammar for the query, so we could do something like ((jim AND NOT haxby AND "important\" paper") OR ds_id:~"^000[3-9]..$" OR url:"example.com") AND metadata:non AND metadata[ex1,ex2]:"specific data" AND metadata[extractor2]:data so composition of booleans and queries within specific extractors etc

…a extractors

…s expected

Also now committed with fixup pre-commit so isort kicked in etc

codecov · 2024-02-22T01:34:20Z

Codecov Report

Attention: Patch coverage is 97.84173% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 98.67%. Comparing base (2d07cb8) to head (209d424).
Report is 59 commits behind head on master.

Files	Patch %	Lines
datalad_registry/search.py	96.59%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
- Coverage   98.73%   98.67%   -0.06%     
==========================================
  Files          50       52       +2     
  Lines        2368     2492     +124     
==========================================
+ Hits         2338     2459     +121     
- Misses         30       33       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jwodder

I don't know lark, so I just proofread the grammar in the syntax description.

datalad_registry/templates/overview.html

setup.cfg

candleindark

If you are going to refer to this "search" scheme as "query" in the web API, I think it would be more consistent if we replace "search" in the naming and references related to this scheme. We can rename search.py and test_search.py into query.py and test_search.py.

datalad_registry/search.py

…help for search syntax Co-authored-by: Isaac To <[email protected]> Co-authored-by: John T. Wodder II <[email protected]>

yarikoptic · 2024-02-23T20:46:08Z

I saw that coverage reported that not_expr is not covered, started to add the tests and ran into the failing test cases:

        ("NOT handbook", [1, 2, 4]),
        ("datalad AND (NOT handbook)", [2]),

which I cannot figure out why our NOT not working as it should -- just returns empty lists :-/
it happens if we do not specify which db entry to operate on, so we fall into that or_ across all known fields in _get_str_search https://github.com/datalad/datalad-registry/pull/308/files#diff-11394d8200a55a0ad0089b9d62dabe6194d7a14e711147c2b8d63a795b5aa2dbR223

    def _get_str_search(self, arg: Token) -> ColumnElement[bool]:
        value = self._get_str_value(arg)
        return or_(*[f(value) for f in known_fields.values()])

any ideas would be appreciated or otherwise might be worth disabling NOT for such cases or altogether for now.

yarikoptic · 2024-02-23T22:44:31Z

interesting... toying around with dandi datasets, and NOT:

it points to two dandisets where dandi metadata is loaded, could potentially have that AccessRequirements but does not! And if I select it to be limited metadata: field, then I get also all the private ones for which I do not even have any metadata loaded:

candleindark · 2024-02-24T01:55:04Z

    def _get_str_search(self, arg: Token) -> ColumnElement[bool]:
        value = self._get_str_value(arg)
        return or_(*[f(value) for f in known_fields.values()])

Based on the intent of what you want to search, it seems that the the lo

interesting... toying around with dandi datasets, and NOT:

it points to two dandisets where dandi metadata is loaded, could potentially have that AccessRequirements but does not! And if I select it to be limited metadata: field, then I get also all the private ones for which I do not even have any metadata loaded:

For some reason, I can't access your posted images.

yarikoptic · 2024-02-27T14:08:21Z

a user can encounter a situation that a tooltip text can obstruct the view and function of buttons.

yeah, I ran into it often too. Added 1em margin for spaceing it out now.

yarikoptic · 2024-02-27T14:09:17Z

I am leaning to actually generalize DB-specific filter into search here. Pushed that unification to "search" in the web UI/parameters (query)

If this is the end goal for this feature, can we route this feature to /api/v2/dataset-urls/search when we implement the API endpoint for this feature instead of /api/v2/dataset-urls/query which you suggested in our previous zoom session?

yes, we can name it /search as it is even present in #269.

This space should not be there since all JSON data are in compact form in text. That space doesn't exist in `BIDSVersion` specification

A list is not needed here, and an iterator is more efficient.

`filter` is a built-in name. `criteria` is the corresponding parameter that is receiving the value

yarikoptic · 2024-02-28T15:18:49Z

@candleindark I "merged" your PR. all good for a merge/deploy? ;)

…uery

These names are more informative. Additionally, doc strings for these tests are also updated.

These names are appropriate now because of the context provided the containing class

candleindark · 2024-02-22T19:48:36Z

datalad_registry/search.py

+known_fields_RepoUrl_1to1 = ["url", "ds_id", "head", "head_describe", "tags"]
+
+
+def get_ilike_search(model, field: str, value: str):


Suggested change

def get_ilike_search(model, field: str, value: str):

def _get_ilike_expr(model, field: str, value: str):

This name is more consistent with SQLAlchemy's convention. I also feel that this function depends heavily on the context of the containing file and should be made private.

candleindark · 2024-02-22T19:50:58Z

datalad_registry/search.py

+    return getattr(model, field).ilike(_escape_for_ilike(value), escape=escape)
+
+
+def get_metadata_ilike_search(value):


Suggested change

def get_metadata_ilike_search(value):

def _get_metadata_ilike_expr(value):

This name is more consistent with SQLAlchemy's convention. I also feel that this function depends heavily on the context of the containing file and should be made private.

candleindark · 2024-02-22T19:52:32Z

datalad_registry/search.py

+    )
+
+
+def get_branches_ilike_search(value):


Suggested change

def get_branches_ilike_search(value):

def _get_branches_ilike_expr(value):

This name is more consistent with SQLAlchemy's convention. I also feel that this function depends heavily on the context of the containing file and should be made private.

candleindark · 2024-02-22T21:08:02Z

datalad_registry/search.py

+        super().__init__(*args, **kwargs)
+        # Additional initialization here if needed
+
+    def or_search(self, *args) -> ColumnElement[bool]:


Suggested change

def or_search(self, *args) -> ColumnElement[bool]:

def or_exp(self, *args) -> ColumnElement[bool]:

This name is more inline with SQLAlchemy's convention.

candleindark · 2024-02-22T21:08:27Z

datalad_registry/search.py

+        # args will be a list of the arguments to the OR operation
+        return or_(*args)
+
+    def and_search(self, *args) -> ColumnElement[bool]:


Suggested change

def and_search(self, *args) -> ColumnElement[bool]:

def and_exp(self, *args) -> ColumnElement[bool]:

This name is more inline with SQLAlchemy's convention.

those must correspond to the names of lark grammar -- defined above at

https://github.com/datalad/datalad-registry/pull/308/files#diff-11394d8200a55a0ad0089b9d62dabe6194d7a14e711147c2b8d63a795b5aa2dbR82

I don't mind changing here and there but then go ahead change and send PR so we make sure that it works

candleindark · 2024-02-22T21:24:06Z

datalad_registry/search.py

+        # args will be a single-element list containing the NOT argument
+        return not_(arg)
+
+    def get_field_select_search(


Suggested change

def get_field_select_search(

def get_field_select_expr(

This name is more inline with SQLAlchemy's convention.

this and below -- I don't mind but make a complete change/refactoring and send as a PR or a commit making sure that all renamed/works.

candleindark · 2024-02-22T21:39:47Z

datalad_registry/search.py

+            raise ValueError(f"Unexpected number of args: {len(args)} in {args}")
+
+        if field_l.data == "field_select":
+            search = partial(self.get_field_select_search, *field_l.children)


Suggested change

search = partial(self.get_field_select_search, *field_l.children)

get_query_exp = partial(self.get_field_select_search, *field_l.children)

This name is more inline with SQLAlchemy's convention.

candleindark · 2024-02-22T22:04:52Z

datalad_registry/search.py

+        else:
+            raise TypeError(arg.type)
+
+    def _get_str_search(self, arg: Token) -> ColumnElement[bool]:


Suggested change

def _get_str_search(self, arg: Token) -> ColumnElement[bool]:

def _get_str_query_exp(self, arg: Token) -> ColumnElement[bool]:

This name is more consistent the SQLAlChemy convention.

candleindark · 2024-02-28T19:53:39Z

datalad_registry/search.py

+        if not extractors:
+            raise GrammarValueError(
+                f"No extractors were specified in {metadata_extractors_l}"
+            )


I tried, but couldn't come up with a test case for these lines. The coverage of these lines is the only thing I want before merging.

I think they are not even reachable per se since lark grammar wouldn't allow for such a case. So it is more of an AssertionError here in particular. But I take it as a defensive coding to code under assumption we might get there. I would have just added the skip for coverage here.

The class is unneeded for all tests in the containing files are for testing the search functionality

Modify tests organization

candleindark · 2024-02-28T21:14:08Z

As @yarikoptic suggested, let's merge this PR now. The remaining "issues" don't effect the functionalities. They are to be addressed in #320.

yarikoptic added 8 commits February 20, 2024 17:27

WiP: started to hang on the parsed content the sqlalchemy queries

b3079b6

ENH: allow for implicit AND, initial construct for specifying metadat…

be5eb9a

…a extractors

Fix name of metalad_studyminimeta extactor name

4740582

Make search on branches work, add basic tests of queries to perform a…

1df3800

…s expected

progress isort forward in pre-commit

7ada250

More testing, various minor tune ups, testing case insensitive

2751ba7

Also now committed with fixup pre-commit so isort kicked in etc

Replace hardcoded search with the flexible one

f015def

yarikoptic requested review from candleindark and jwodder February 21, 2024 21:02

yarikoptic added the enhancement New feature or request label Feb 21, 2024

yarikoptic added 2 commits February 21, 2024 16:10

ENH: Add a modal window describing the syntax for new filter

395b707

ENH: add codespell to pre-commit since we otherwise let typos sneak in

e2671f9

yarikoptic force-pushed the enh-search branch from 61144ea to e2671f9 Compare February 21, 2024 21:11

Add few basic type annotations and ignore 2 in search for now

62d884c

yarikoptic force-pushed the enh-search branch from ffd5549 to 62d884c Compare February 22, 2024 01:26

jwodder requested changes Feb 22, 2024

View reviewed changes

candleindark requested changes Feb 22, 2024

View reviewed changes

setup.cfg Show resolved Hide resolved

candleindark reviewed Feb 22, 2024

View reviewed changes

datalad_registry/search.py Outdated Show resolved Hide resolved

yarikoptic marked this pull request as ready for review February 22, 2024 21:57

yarikoptic and others added 3 commits February 22, 2024 17:02

Add lark==1.1.9 to requirements.txt

b58bd9d

Apply suggestions from code review: primarily wording tuneups of the …

4b40b5f

…help for search syntax Co-authored-by: Isaac To <[email protected]> Co-authored-by: John T. Wodder II <[email protected]>

Add logging for search + add test cases for NOT -- two still fail :-/

749169f

candleindark mentioned this pull request Feb 24, 2024

Handle NULL column values correctly in ilike queries #315

Merged

candleindark added 2 commits February 24, 2024 08:10

Reformat code

44ddcd9

Handle null column values correctly in ilike queries

795e966

Add 1 em margin on top of table to let tips not occlude buttons

564b68a

yarikoptic and others added 11 commits February 27, 2024 09:10

Fix the test for renamed filter -> query

8db70a3

Run testing for all PRs

9f5741d

ENH: add a tricky case for searching for a string within JSON record

f2fa21c

Rename test since it is no longer just smoke

27a656e

Remove space in example for restrictive metadata search

31b5803

This space should not be there since all JSON data are in compact form in text. That space doesn't exist in `BIDSVersion` specification

Add explanation for the last query syntax example

be2ffbf

Replace list with iterator

2e79584

A list is not needed here, and an iterator is more efficient.

Update overview endpoint to use modern SQLAlchemy syntax

19fe126

Rename variable

84c8699

`filter` is a built-in name. `criteria` is the corresponding parameter that is receiving the value

Update test_search_complex_smoke to use modern syntax of SQLAlchemy

bb50a71

Bring back space in the search "JSON string" example

014aa85

yarikoptic and others added 5 commits February 28, 2024 10:41

Improve tests of overview -- test for ERROR to be given for a wrong q…

c38fdba

…uery

Rename tests for the overview page

f2ee385

These names are more informative. Additionally, doc strings for these tests are also updated.

Group tests for the search function into a class

dfaa327

Rename the tests for the search function

8e85071

These names are appropriate now because of the context provided the containing class

Provide doc strings for tests of search functionality

130cb92

candleindark mentioned this pull request Feb 28, 2024

Modify tests organization #319

Merged

candleindark reviewed Feb 28, 2024

View reviewed changes

candleindark and others added 3 commits February 28, 2024 12:36

Move tests for search out of a class

9b4fb25

The class is unneeded for all tests in the containing files are for testing the search functionality

Specify purpose of the test_search.py module in comment

1bdd6a3

Merge pull request #319 from candleindark/enh-search-review

209d424

Modify tests organization

candleindark mentioned this pull request Feb 28, 2024

Create a PR for possible minor improvements on #308 #320

Open

candleindark merged commit 720d08a into master Feb 28, 2024
7 of 9 checks passed

candleindark deleted the enh-search branch February 28, 2024 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic but flexible query language #308

Add basic but flexible query language #308

yarikoptic commented Feb 21, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

jwodder left a comment

candleindark left a comment •

edited

Loading

yarikoptic commented Feb 23, 2024

yarikoptic commented Feb 23, 2024

candleindark commented Feb 24, 2024

yarikoptic commented Feb 27, 2024

yarikoptic commented Feb 27, 2024

yarikoptic commented Feb 28, 2024

candleindark Feb 22, 2024

candleindark Feb 22, 2024

candleindark Feb 22, 2024

candleindark Feb 22, 2024

candleindark Feb 22, 2024

yarikoptic Feb 28, 2024

candleindark Feb 22, 2024

yarikoptic Feb 28, 2024

candleindark Feb 22, 2024

candleindark Feb 22, 2024

candleindark Feb 28, 2024

yarikoptic Feb 28, 2024

candleindark commented Feb 28, 2024

		known_fields_RepoUrl_1to1 = ["url", "ds_id", "head", "head_describe", "tags"]


		def get_ilike_search(model, field: str, value: str):

	def get_ilike_search(model, field: str, value: str):
	def _get_ilike_expr(model, field: str, value: str):

		return getattr(model, field).ilike(_escape_for_ilike(value), escape=escape)


		def get_metadata_ilike_search(value):

	def get_metadata_ilike_search(value):
	def _get_metadata_ilike_expr(value):

	def get_branches_ilike_search(value):
	def _get_branches_ilike_expr(value):

	def or_search(self, *args) -> ColumnElement[bool]:
	def or_exp(self, *args) -> ColumnElement[bool]:

	def and_search(self, *args) -> ColumnElement[bool]:
	def and_exp(self, *args) -> ColumnElement[bool]:

	search = partial(self.get_field_select_search, *field_l.children)
	get_query_exp = partial(self.get_field_select_search, *field_l.children)

	def _get_str_search(self, arg: Token) -> ColumnElement[bool]:
	def _get_str_query_exp(self, arg: Token) -> ColumnElement[bool]:

Add basic but flexible query language #308

Add basic but flexible query language #308

Conversation

yarikoptic commented Feb 21, 2024 • edited Loading

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

jwodder left a comment

Choose a reason for hiding this comment

candleindark left a comment • edited Loading

Choose a reason for hiding this comment

yarikoptic commented Feb 23, 2024

yarikoptic commented Feb 23, 2024

candleindark commented Feb 24, 2024

yarikoptic commented Feb 27, 2024

yarikoptic commented Feb 27, 2024

yarikoptic commented Feb 28, 2024

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

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

candleindark commented Feb 28, 2024

yarikoptic commented Feb 21, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

candleindark left a comment •

edited

Loading