Skip to content
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

MOD-4152: Return JSON from multi value #3060

Merged
merged 34 commits into from
Oct 27, 2022
Merged

MOD-4152: Return JSON from multi value #3060

merged 34 commits into from
Oct 27, 2022

Conversation

oshadmi
Copy link
Collaborator

@oshadmi oshadmi commented Sep 2, 2022

Enable to RETURN multi values,
Such as multi value attributes obtained from a JSONPath with wildcards, etc., for example, $..field[*].
The entire JSON string representation of the values should be returned (and not only the first value)

MOD-4152
MOD-4142
MOD-4267

Requires

More details

For consistency and to decouple the query from the returned value, we always return a top-level array:

  • If path leads to a single value, e.g., the number 5, will return '[5]', and an array ["a", "b"] will return [["a", "b"]]
  • If path leads to multiple values, e.g., all items inside some array $.arr[*], an array ["a", "b"] will return ["a", "b"]

Client/user code would need to parse the returned JSON (also needed before this PR as long as we return bulk string)
and now, with this PR, to iterate the returned array (even when it has only a single value)

Backward compatibility

  • Fixing the return value to always return an array may break many applications and therefore it is currently disabled by default
    • A usage of an "API Version" concept is needed
    • Since a configuration parameter is not sufficient for the cloud, it could either be set globally in a module configuration parameter, or set explicitly in each FT.SEARCH or FT.AGGREGATE command
  • Since we already have the DIALECT parameter in FT.SEARCH and FT.AGGREGATE command, and the DEFAULT_DIALECT configuration parameter, we utilize them
    • In order to enable it, set the configuration parameter DEFAULT_DIALECT 3, or explicitly use DIALECT 3 in FT.SEARCH or FT.AGGREGATE command

Copy link
Contributor

@ashtul ashtul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments

src/rlookup.c Outdated Show resolved Hide resolved
src/rlookup.c Outdated Show resolved Hide resolved
src/rlookup.c Outdated Show resolved Hide resolved
ashtul
ashtul previously approved these changes Sep 5, 2022
Copy link
Contributor

@ashtul ashtul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/rlookup.c Outdated Show resolved Hide resolved
src/rlookup.c Outdated Show resolved Hide resolved
ashtul
ashtul previously approved these changes Sep 5, 2022
@MeirShpilraien
Copy link
Collaborator

@K-Jo please approve this API changes.

@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Base: 83.01% // Head: 82.49% // Decreases project coverage by -0.51% ⚠️

Coverage data is based on head (0831bd4) compared to base (e9927e9).
Patch coverage: 30.52% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3060      +/-   ##
==========================================
- Coverage   83.01%   82.49%   -0.52%     
==========================================
  Files         181      181              
  Lines       30031    30105      +74     
==========================================
- Hits        24929    24834      -95     
- Misses       5102     5271     +169     
Impacted Files Coverage Δ
src/config.h 100.00% <ø> (ø)
src/rlookup.h 100.00% <ø> (ø)
src/sortable.c 61.71% <0.00%> (-1.49%) ⬇️
src/value.c 63.20% <0.00%> (-3.77%) ⬇️
src/value.h 75.94% <0.00%> (-1.98%) ⬇️
src/document.c 73.94% <27.27%> (-0.97%) ⬇️
src/json.c 89.96% <28.57%> (-1.34%) ⬇️
src/rlookup.c 76.77% <48.57%> (-2.33%) ⬇️
src/aggregate/aggregate_exec.c 95.96% <57.14%> (-0.76%) ⬇️
src/document_basic.c 85.06% <66.66%> (-0.23%) ⬇️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -79,6 +79,7 @@ def testPrefix2(env):
env.assertIn('that:foo', res)
env.assertIn('this:foo', res)

@no_asan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unbound name: Name no_asan is used but not defined in the current scope.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

src/trie/trie.c Outdated Show resolved Hide resolved
src/rlookup.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments

src/json.c Show resolved Hide resolved
Copy link
Contributor

@ashtul ashtul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oshadmi oshadmi merged commit ba041f4 into master Oct 27, 2022
@oshadmi oshadmi deleted the omer_multi_projection branch October 27, 2022 08:36
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.

None yet

5 participants