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

Add support for FT.SEARCH NOCONTENT #2610

Merged
merged 7 commits into from
Sep 18, 2023
Merged

Add support for FT.SEARCH NOCONTENT #2610

merged 7 commits into from
Sep 18, 2023

Conversation

brettburch
Copy link
Contributor

@brettburch brettburch commented Sep 8, 2023

Description

This PR implements the TODO item at

// NOCONTENT?: true; TODO

Similar to #2488


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@leibale
Copy link
Collaborator

leibale commented Sep 12, 2023

@brettburch thanks for contributing! but... if I remember correctly the NOCONTENT option changes the reply structure, which will break transformReply. You can use RETURN: [] to achieve (almost) the same thing..

The NOCONTENT option should be added as a "special command" (see AGGREGATE_WITHCURSOR for example).

@brettburch
Copy link
Contributor Author

if I remember correctly the NOCONTENT option changes the reply structure, which will break transformReply. You can use RETURN: [] to achieve (almost) the same thing..

Ahh, yes, that's true. Only the keys are returned with this option. To your point, for my initial research on this I did:

import {
  SearchRawReply,
  transformArguments,
  transformReply as transformSearchReply,
} from '@redis/search/dist/commands/SEARCH';

...

  async search(
    ...
    options?: SearchOptions,
    additionalParams?: { NOCONTENT?: true },
  ): Promise<SearchReply> {
    const allArgs = transformArguments(index, query, options);
    if (additionalParams) {
      if (additionalParams.NOCONTENT) {
        allArgs.push('NOCONTENT');
      }
    }

    const rawReply = await this.redisClient.sendCommand<SearchRawReply>(allArgs);
    return transformSearchReply(rawReply, additionalParams?.NOCONTENT || false);
  }

In this case the documents that are returned have id set but no value.

The NOCONTENT option should be added as a "special command" (see AGGREGATE_WITHCURSOR for example).

Sounds good. I will move towards a SEARCH_WITHNOCONTENT implementation.

Thanks for the review!

@leibale
Copy link
Collaborator

leibale commented Sep 12, 2023

@brettburch name it SEARCH_NOCONTENT/searchNoContent, not SEARCH_WITHNOCONTENT
Thanks again! :)

edit: I totally forgot about the withoutDocuments thing.. since value is already optional, just reuse the same boolean...
The way "preserve" works is - if the args array has a preserve argument, it'll be sent to the transformReply as a second argument (a very hacky solution for commands that need to save some of the arguments to transform the reply).

So add args.preserve = true; in the if (options?.NOCONTENT) block, and that should work. Just make sure to add a test for that... :)

another edit:
actually we should move it to SEARCH_NOCONTENT/searchNoContent since when withoutDocuments is true it'll create an empty object for each document

value: withoutDocuments ? Object.create(null) : documentValue(reply[i++])

which I don't think we should do when NOCONTENT is used. On top of that, instead of returning

type Reply = {
  total: number;
  documents: Array<{ id: string, document: ... }>;
};

we should return

type Reply = {
  total: number;
  documents: Array<string>;
};

Sorry for the confusion...

@leibale
Copy link
Collaborator

leibale commented Sep 12, 2023

@brettburch see the edit, sorry about the confusion...

@brettburch
Copy link
Contributor Author

@leibale I believe I understood your feedback, but let me know if there's anything else to update here.

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4976231) 95.71% compared to head (6607e4f) 95.71%.
Report is 4 commits behind head on master.

❗ Current head 6607e4f differs from pull request most recent head 4d45e5c. Consider uploading reports for the commit 4d45e5c to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2610   +/-   ##
=======================================
  Coverage   95.71%   95.71%           
=======================================
  Files         456      457    +1     
  Lines        4523     4530    +7     
  Branches      506      506           
=======================================
+ Hits         4329     4336    +7     
  Misses        127      127           
  Partials       67       67           
Files Changed Coverage Δ
packages/search/lib/commands/SEARCH.ts 82.60% <ø> (ø)
packages/search/lib/commands/SEARCH_NOCONTENT.ts 100.00% <100.00%> (ø)

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

@brettburch
Copy link
Contributor Author

@leibale I see the node 14 test runs are failing. Is node 14 still supported? Only 18 and 20 are currently active/maintained. If 14 is supported in this package I can investigate the test failures.

@leibale
Copy link
Collaborator

leibale commented Sep 15, 2023

@brettburch I just did not remove them from the tests matrix yet.. I'll take care of it this week.

I think this PR is ready.. :)

@leibale leibale changed the title Add support for NOCONTENT in FT.SEARCH Add support for FT.SEARCH NOCONTENT Sep 18, 2023
@leibale leibale merged commit a217cc1 into redis:master Sep 18, 2023
10 checks passed
@leibale
Copy link
Collaborator

leibale commented Sep 19, 2023

@brettburch [email protected]/@redis/[email protected] is on npm 🎉

@brettburch
Copy link
Contributor Author

Great, thanks for the update @leibale ! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants