Add mongo length operator functionality with length aliases #222

ml-evs · 2020-03-10T20:10:22Z

This PR is a WIP/proof of concept for the introduction of "length aliases" to attempt to fix the issues in #86.

This PR adds a post-processing function to the MongoTransformer which recursively descends into the transformed filter and tries to change any {"x": {"$size": 3}} queries with the value of a value of corresponding aliased field, e.g. elements->nelements. The benefits are minimal when just performing LENGTH = value queries (as $size can do this rapidly anyway), but this will greatly speed up LENGTH > 3 queries, which mongo does not support directly (see #86 discussion). Length operator queries that do not have a defined length alias will now fallback to using the existence test approach, i.e. LENGTH > 3 becomes "does element 4 exist?".

Extra changes needed to support this:

splitting of entry_collections into submodules (entry_collections.entry_collections and entry_collections.mongo).
addition of LENGTH_ALIASES attribute to mappers and config.
addition of PROVIDER_FIELDS to mapper to mirror other aliases.
use of $exists for fallback length operations
moved all aliasing code out of collections and into the transformer, as post-processing
all of the spec-defined length aliases are now part of the default StructureMapper, but will be ignored unless a Transformer makes use of them.

codecov · 2020-03-10T20:16:50Z

Codecov Report

Merging #222 into master will increase coverage by 0.42%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   87.11%   87.53%   +0.42%     
==========================================
  Files          43       45       +2     
  Lines        1862     1941      +79     
==========================================
+ Hits         1622     1699      +77     
- Misses        240      242       +2

Flag	Coverage Δ
#unittests	`87.53% <95.58%> (+0.42%)`	⬆️

Impacted Files	Coverage Δ
...made/server/entry_collections/entry_collections.py	`88.88% <88.88%> (ø)`
optimade/filtertransformers/mongo.py	`96.36% <97.29%> (+0.71%)`	⬆️
optimade/server/config.py	`100.00% <100.00%> (ø)`
optimade/server/entry_collections/__init__.py	`100.00% <100.00%> (ø)`
optimade/server/entry_collections/mongo.py	`86.73% <100.00%> (ø)`
optimade/server/mappers/entries.py	`98.11% <100.00%> (+0.33%)`	⬆️
optimade/server/mappers/structures.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7768f62...9be0f61. Read the comment docs.

CasperWA

Interesting implementation. Very nice. I have some minor suggested changes.

optimade/server/mappers/entries.py

ml-evs · 2020-03-13T19:45:59Z

Interesting implementation. Very nice. I have some minor suggested changes.

Are you happy with the proposed structure then, or would you prefer it in MongoCollection? I'm on the fence myself...

ml-evs · 2020-03-13T21:54:12Z

I've just realised another feature that could use this approach, filtering on relationships. I've implemented this on my branch ml-evs/relationships_filtering. I can make a PR once we're ironed out this one.

CasperWA · 2020-03-17T14:24:30Z

Interesting implementation. Very nice. I have some minor suggested changes.

Are you happy with the proposed structure then, or would you prefer it in MongoCollection? I'm on the fence myself...

Yeah. I see what you mean. I don't know. I would probably prefer it in the collection class, however, it may also be useful for other backends? But I'm not entirely sure.

ml-evs · 2020-03-17T14:27:44Z

Interesting implementation. Very nice. I have some minor suggested changes.

Are you happy with the proposed structure then, or would you prefer it in MongoCollection? I'm on the fence myself...

Yeah. I see what you mean. I don't know. I would probably prefer it in the collection class, however, it may also be useful for other backends? But I'm not entirely sure.

I've completely rewritten this process to make it more general across different post-processing operations, but think the actual descent into the query is going to very mongo specific. As it is, we can definitely lift this out and put it into the collection class.

CasperWA · 2020-03-17T14:31:09Z

(...) As it is, we can definitely lift this out and put it into the collection class.

ml-evs · 2020-03-17T14:33:59Z

This is perhaps the worst feature GH have ever added, the reddit-ification of the "workplace" continues unbounded

shyamd

Looks good to me.

ml-evs · 2020-03-26T13:42:25Z

I'll give @CasperWA a while (~week) to look before merging this.

CasperWA

Looking awesome, thanks @ml-evs!

Only a few minor comments.
I am truly enjoying that the Collection has been trimmed a bit, and the Transformer can handle all transformation-related aspects of the query, instead of having a bit here and there.

optimade/filtertransformers/mongo.py

optimade/server/mappers/entries.py

tests/filtertransformers/test_mongo.py

CasperWA · 2020-03-30T13:51:17Z

tests/filtertransformers/test_mongo.py

+
+    def test_unaliased_length_operator(self):
+        transformer = MongoTransformer()
+        parser = LarkParser(version=self.version, variant=self.variant)


Same here as the previous comment. And even the transformer doesn't need to be redefined, since it should be t from line 16?

The transformer in these tests needs to take a mapper as an argument, so the same one can't be reused. This means the parser also can't be reused (its out of scope in the methods, we only expose the transform method which is tied to the transformer).

If you see the def setUp(), which runs just prior of this test method, why wouldn't that suffice for the tests in this method?

The transformer in these tests needs to take a mapper as an argument (...)

But in this exact test method, the transformer doesn't take a mapper as an argument? I am confused :( I see that it makes sense to instantiate a new transformer for the other test methods you added, but not for this one.

And how is the parser out of scope? It doesn't depend on the transformer as far as I know? Or? :|

The transformer in these tests needs to take a mapper as an argument, so the same one can't be reused. This means the parser also can't be reused (its out of scope in the methods, we only expose the transform method which is tied to the transformer).

AAAHHH I see the whole problem now - sorry!! Yeah, we set both the parser and transformer in the setUp method, but only keep the transform in the instance..... right, right, right.

So would it make sense then to expose the instantiated transformer and parser?
Or maybe create a transform_mapped lambda function?

Also, I think the setUp() should be setUpClass(), right? Is there a need to instantiate this before each test method?

Replied to the wrong comment initially, sorry! I've amended my commit with your suggestion

So would it make sense then to expose the instantiated transformer and parser?

The other two new tests can't use this anyway, so I think it's now fine as is

Cool. I'll stop poking at this.

(although, just tested locally changing this to setUpClass(), and it works only if one then makes transform an actual separate class method, and consequently "saves" the instantiated parser and transformer as class attributes.)

tests/filtertransformers/test_mongo.py

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

CasperWA

C'est magnifique !

Thanks for the additions and updates. I think this quite an elegant solution in the end 👍
Please merge at your leisure.

ml-evs added enhancement New feature or request priority/medium Issue or PR with a consensus of medium priority transformer/MongoDB Related to filter transformer for MongoDB labels Mar 10, 2020

CasperWA requested changes Mar 11, 2020

View reviewed changes

optimade/server/mappers/entries.py Outdated Show resolved Hide resolved

optimade/server/mappers/entries.py Outdated Show resolved Hide resolved

ml-evs mentioned this pull request Mar 16, 2020

Enable filtering on relationships with mongo #234

Merged

ml-evs force-pushed the ml-evs/length_operator_queries branch from 0d03180 to b0de4ad Compare March 17, 2020 14:10

ml-evs force-pushed the ml-evs/length_operator_queries branch 2 times, most recently from bdc1205 to b4f528f Compare March 19, 2020 14:38

ml-evs force-pushed the ml-evs/length_operator_queries branch 3 times, most recently from 2e034a1 to 51cf0cd Compare March 25, 2020 11:21

ml-evs requested review from CasperWA, shyamd and fekad March 25, 2020 12:26

shyamd previously approved these changes Mar 25, 2020

View reviewed changes

ml-evs added 7 commits March 28, 2020 17:51

Added length aliases to mappers and config

a477e38

Added general recursive postprocessing to MongoTransformer

6c503cd

Added length alias postprocessing

ea6ec97

Added tests for length aliasing

314d9c9

Split entry_collections into two submodules

b9e30be

Move all filter aliasing out of collection and into transformer

fc97637

Added spec-defined length aliases to StructureMapper

3f90705

ml-evs added 12 commits March 28, 2020 17:51

Tidy length aliases slightly

961b8ec

Enable length operator queries with tests for existence

c17bcb6

Added transformer tests for length operator queries

d407373

Move all error reporting to grammar parsing

3af9627

Added tests for aliased/unaliased length operators

a4fe842

Enabled server length operator tests

3f8e762

Some minor import tweaks

f797690

Added configurable length aliases

a3293ca

Added tests to handle circular aliases

2ad2df6

Added provider fields specification to mapper

ff965be

Tests for length aliases provider fields

a1847fc

Extend alias checks to length aliases

8d176a8

ml-evs dismissed shyamd’s stale review via 8d176a8 March 28, 2020 17:52

ml-evs force-pushed the ml-evs/length_operator_queries branch from 280a1e7 to 8d176a8 Compare March 28, 2020 17:52

CasperWA reviewed Mar 30, 2020

View reviewed changes

ml-evs requested a review from CasperWA March 30, 2020 16:19

Suggestions from code review

9be0f61

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

ml-evs force-pushed the ml-evs/length_operator_queries branch from 7884dea to 9be0f61 Compare March 30, 2020 16:22

CasperWA approved these changes Mar 30, 2020

View reviewed changes

ml-evs merged commit 6d9a179 into master Mar 30, 2020

ml-evs deleted the ml-evs/length_operator_queries branch March 30, 2020 16:49

ml-evs mentioned this pull request Mar 31, 2020

Implement LENGTH in query #86

Closed

CasperWA mentioned this pull request Apr 22, 2020

Up to v0.8.0 #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mongo length operator functionality with length aliases #222

Add mongo length operator functionality with length aliases #222

ml-evs commented Mar 10, 2020 •

edited

codecov bot commented Mar 10, 2020 •

edited

CasperWA left a comment

ml-evs commented Mar 13, 2020

ml-evs commented Mar 13, 2020 •

edited

CasperWA commented Mar 17, 2020

ml-evs commented Mar 17, 2020

CasperWA commented Mar 17, 2020

ml-evs commented Mar 17, 2020

shyamd left a comment

ml-evs commented Mar 26, 2020

CasperWA left a comment

CasperWA Mar 30, 2020

ml-evs Mar 30, 2020

CasperWA Mar 30, 2020

CasperWA Mar 30, 2020

CasperWA Mar 30, 2020 •

edited

ml-evs Mar 30, 2020

ml-evs Mar 30, 2020

CasperWA Mar 30, 2020 •

edited

CasperWA left a comment •

edited

Add mongo length operator functionality with length aliases #222

Add mongo length operator functionality with length aliases #222

Conversation

ml-evs commented Mar 10, 2020 • edited

codecov bot commented Mar 10, 2020 • edited

Codecov Report

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Mar 13, 2020

ml-evs commented Mar 13, 2020 • edited

CasperWA commented Mar 17, 2020

ml-evs commented Mar 17, 2020

CasperWA commented Mar 17, 2020

ml-evs commented Mar 17, 2020

shyamd left a comment

Choose a reason for hiding this comment

ml-evs commented Mar 26, 2020

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA Mar 30, 2020

Choose a reason for hiding this comment

ml-evs Mar 30, 2020

Choose a reason for hiding this comment

CasperWA Mar 30, 2020

Choose a reason for hiding this comment

CasperWA Mar 30, 2020

Choose a reason for hiding this comment

CasperWA Mar 30, 2020 • edited

Choose a reason for hiding this comment

ml-evs Mar 30, 2020

Choose a reason for hiding this comment

ml-evs Mar 30, 2020

Choose a reason for hiding this comment

CasperWA Mar 30, 2020 • edited

Choose a reason for hiding this comment

CasperWA left a comment • edited

Choose a reason for hiding this comment

ml-evs commented Mar 10, 2020 •

edited

codecov bot commented Mar 10, 2020 •

edited

ml-evs commented Mar 13, 2020 •

edited

CasperWA Mar 30, 2020 •

edited

CasperWA Mar 30, 2020 •

edited

CasperWA left a comment •

edited