New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The keyword "exists" can now search for array values #17

Merged
merged 11 commits into from Sep 25, 2018

Conversation

Projects
None yet
4 participants
@scottinet
Member

scottinet commented Sep 20, 2018

Description

Allow Koncorde users to search for the existence of array values.

A new "array value" syntax has been added to the already existing nested field syntax:

  • Existing nested field syntax: foo.bar.baz => looks for the following object: foo: {bar: {baz: ...}}}
  • Array value syntax: foo["bar"] => looks for a string value bar in the array foo.

The new array syntax allows looking for any scalar: boolean, numbers, null and strings. For the latter, the string value must be enclosed in double quotes, as in JSON syntax.

It is possible to mix nested field + array value syntaxes: foo.bar.baz[42]

It is also possible to escape the array syntax, allowing to search for field keys named, for instance, foo[bar] (e.g. { "foo[bar]": true }): foo\\[bar\\]

This new syntax can now be used with the exists and missing keywords, making it easy to understand what it does: {exists: {field: 'foo["bar"]' }}

Also, the exists/missing keyword syntax has been simplified, from {[exists|missing]: { field: '<value>' }} to {[exists|missing]: '<value>'}
The old syntax remains available for backward compatibility.

Documentation PR: kuzzleio/documentation#532

How should this be manually tested?

Unit tests are already VERY thorough, but feel free to play with the new syntax :)

Other changes

I was unable to get adequate performances while looking for array values, so I had to take a look at the overall Koncorde performances. I would never have thought that duplicating a Uint8Array would be SOOO slow: getting rid of that duplication in TestTables object allowed a (very, very, very) substantial increase of performances. This is quite insane, as TypedArray's are meant to be low-level, undecorated objects, added to Javascript especially for high performance bytes manipulation. I was naive to blindly trust these assumptions without checking V8 performances first, it seems...
Check the new benchmarks!

Also, I removed usages of the sorted-array NPM library: it was useful in Node.js 4, but now Set and Map are far better and much faster.

Boyscout

  • Simplified a few keyword storages, removing unnecessary object levels.
  • add a new Syntax paragraph to each keyword documentation
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 20, 2018

Codecov Report

Merging #17 into 1-dev will increase coverage by 0.03%.
The diff coverage is 97.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##            1-dev      #17      +/-   ##
==========================================
+ Coverage   95.15%   95.18%   +0.03%     
==========================================
  Files          34       33       -1     
  Lines        1115     1081      -34     
==========================================
- Hits         1061     1029      -32     
+ Misses         54       52       -2
Impacted Files Coverage Δ
lib/match/matchExists.js 100% <100%> (ø) ⬆️
lib/match/matchNotGeospatial.js 100% <100%> (ø) ⬆️
lib/match/matchNotEquals.js 100% <100%> (ø) ⬆️
lib/match/matchGeospatial.js 100% <100%> (ø) ⬆️
lib/match/matchRegexp.js 100% <100%> (ø) ⬆️
lib/match/matchNotExists.js 100% <100%> (ø) ⬆️
lib/match/matchNotRegexp.js 100% <100%> (ø) ⬆️
lib/match/matchEquals.js 100% <100%> (ø) ⬆️
lib/match/index.js 100% <100%> (ø) ⬆️
lib/storage/objects/testTable.js 100% <100%> (ø) ⬆️
... and 10 more

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 7603693...5363863. Read the comment docs.

codecov-io commented Sep 20, 2018

Codecov Report

Merging #17 into 1-dev will increase coverage by 0.03%.
The diff coverage is 97.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##            1-dev      #17      +/-   ##
==========================================
+ Coverage   95.15%   95.18%   +0.03%     
==========================================
  Files          34       33       -1     
  Lines        1115     1081      -34     
==========================================
- Hits         1061     1029      -32     
+ Misses         54       52       -2
Impacted Files Coverage Δ
lib/match/matchExists.js 100% <100%> (ø) ⬆️
lib/match/matchNotGeospatial.js 100% <100%> (ø) ⬆️
lib/match/matchNotEquals.js 100% <100%> (ø) ⬆️
lib/match/matchGeospatial.js 100% <100%> (ø) ⬆️
lib/match/matchRegexp.js 100% <100%> (ø) ⬆️
lib/match/matchNotExists.js 100% <100%> (ø) ⬆️
lib/match/matchNotRegexp.js 100% <100%> (ø) ⬆️
lib/match/matchEquals.js 100% <100%> (ø) ⬆️
lib/match/index.js 100% <100%> (ø) ⬆️
lib/storage/objects/testTable.js 100% <100%> (ø) ⬆️
... and 10 more

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 7603693...5363863. Read the comment docs.

@scottinet

Sorry for the lengthy PR, I had to improve many parts of Koncorde to make performances acceptable. I probably should have separated the perf. optimizations from the new array value syntax, but I didn't 😞

So here are a few pointers to the interesting changes I made. You might want to focus on these.

return Bluebird.resolve(filter);
}
// Syntax: {exists: '<field name>'}

This comment has been minimized.

@scottinet

scottinet Sep 20, 2018

Member

New exists syntax here.

@scottinet

scottinet Sep 20, 2018

Member

New exists syntax here.

@@ -772,4 +780,52 @@ function mustBeNonEmptyArray(filter, keyword, field) {
return Bluebird.resolve();
}
/**

This comment has been minimized.

@scottinet

scottinet Sep 20, 2018

Member

Start of the function parsing the new array value syntax.

@scottinet

scottinet Sep 20, 2018

Member

Start of the function parsing the new array value syntax.

scottinet added some commits Sep 21, 2018

@scottinet scottinet referenced this pull request Sep 25, 2018

Merged

Optimize filter removal #18

@Aschen

Aschen approved these changes Sep 25, 2018

@Aschen Aschen merged commit 65b1cae into 1-dev Sep 25, 2018

3 checks passed

codecov/project 95.18% (+0.03%) compared to 7603693
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Aschen Aschen deleted the KZL-504/exists-handle-arrays branch Sep 25, 2018

@scottinet scottinet referenced this pull request Sep 25, 2018

Merged

Release 1.2.0 #19

scottinet added a commit that referenced this pull request Sep 25, 2018

Release 1.2.0 (#19)
* dependencies update (#15)

* fix #13

* The keyword "exists" can now search for array values (#17)

* document array value syntax

* unit tests update + bugfixes

* fix broken test

* forgot to add the new file to git -_-

* use a Set to store field=>operand keys

* fix array value description

* add missing tests (oops)

* even better performances

* apply @amaret comment

* Optimize filter removal (#18)

* document array value syntax

* wip

* unit tests update + bugfixes

* fix broken test

* forgot to add the new file to git -_-

* use a Set to store field=>operand keys

* fix array value description

* add missing tests (oops)

* even better performances

* (wip)

* further simplification + unit tests updates

* modernize, simplify, optimize

* leftovers

* add LGTM badges

* Release 1.2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment