-
Notifications
You must be signed in to change notification settings - Fork 54
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
Java: FT.PROFILE
#2473
base: release-1.2
Are you sure you want to change the base?
Java: FT.PROFILE
#2473
Conversation
6641d4d
to
6b23e6b
Compare
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
94ae1d3
to
f1e346e
Compare
…t-profile-valkey-424 Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
…t-profile-valkey-424 Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
public FTProfileOptions( | ||
@NonNull QueryType queryType, @NonNull GlideString[] commandLine, boolean limited) { | ||
this.queryType = queryType; | ||
this.query = commandLine; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called commandLine? It should be query.
Same for all occurrences of the variable in the entire file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is a complete command line for aggregate/search except first 2 args
* @param query The query itself. | ||
* @param options {@link FT#aggregate} options. | ||
*/ | ||
public FTProfileOptions(@NonNull String query, @NonNull FTAggregateOptions options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pass FTAggregateOptions? I don't see anything in the documentation about this. In the documentation, the query option is the query string passed into the FT.SEARCH or FT.AGGREGATE.
I think maybe the documentation does not mention this clearly, but it is possible to pass everything after the query field from FT.SEARCH/FT.AGGREGATE to the FT.PROFILE query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command accepts all arguments of aggregate/search and does the same action + profiling.
* @param query The query itself. | ||
* @param options {@link FT#search} options. | ||
*/ | ||
public FTProfileOptions(@NonNull String query, @NonNull FTSearchOptions options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Why do we need FTSearchOptions?
@@ -1472,6 +1511,14 @@ pub(crate) fn expected_type_for_cmd(cmd: &Cmd) -> Option<ExpectedReturnType> { | |||
}), | |||
b"FT.AGGREGATE" => Some(ExpectedReturnType::FTAggregateReturnType), | |||
b"FT.SEARCH" => Some(ExpectedReturnType::FTSearchReturnType), | |||
// TODO replace with tuple | |||
b"FT.PROFILE" => Some(ExpectedReturnType::FTProfileReturnType( | |||
if cmd.arg_idx(2).is_some_and(|a| a == b"SEARCH") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting solution - good re-use of code
* @param queryType The query type. | ||
* @param commandLine Command arguments (not including index name). | ||
*/ | ||
public FTProfileOptions(@NonNull QueryType queryType, @NonNull GlideString[] commandLine) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you consider using a @Builder
for options? There are a lot of constructor combinations, and the QueryType could be abstracted behind a method instead of an enum.
This would also make adding a query and options simple, and the command line list could finally be constructor on .build() instead of using these constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, where is this array of commandLine/Strings coming from? It's not in the API on either Redis or MemDB.
Shouldn't query
be part of the FT.PROFILE method signature (required) and not included in the Options? I was expecting to see something like this for the FT.PROFILE method signatures:
FT.PROFILE(String query, QueryType queryType);
FT.PROFILE(String query, QueryType queryType, boolean limited);
FT.PROFILE(String query, FTAggregateOptions aggregateOptions);
FT.PROFILE(String query, FTAggregateOptions aggregateOptions, boolean limited);
FT.PROFILE(String query, FTSearchOptions searchOptions);
FT.PROFILE(String query, FTSearchOptions searchOptions, boolean limited);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using a @builder for options
QueryType
and commandLine
are both mandatory parameters, but QueryType
and WhateverOptions
are mutually exclusive. We need a smart builder to restrict which combination of parameters are allowed.
It's not in the API
It is, but docs aren't clear. PROFILE
accepts the entire command line of AGGREGATE
/SEARCH
with all options, not just query.
Shouldn't
query
be part of theFT.SEARCH
I think you considered FT.PROFILE
. Yes, it is a good option, but keep in mind gs
variant, which duplicates the API. We need to update FT.SEARCH
and FT.AGGREGATE
to align (and in python too?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. If you make the commandline constructors private, then only expose the QueryType | AggregateOptions | SearchOptions, I think that would be perfect.
You can do that with a builder, or with constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove them completely. I added this to allow a user to use FT.PROFILE
with a CLI command line if it is stored somewhere in notes.
Issue link
#2428
Checklist
Before submitting the PR make sure the following are checked: