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

query params #152

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

query params #152

wants to merge 7 commits into from

Conversation

DvirDukhan
Copy link
Contributor

@DvirDukhan DvirDukhan commented Oct 3, 2021

Query params for search functionality
tested against omer-query-params branch

Copy link

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

What about supplying the parameters dict when executing a query (in Client.search or Client.aggregate)
So the parameter names are used in a query, and the query could be executed multiple times, each time with specific param values.

self._params = params
return self

def add_param(self, param_name:str, value:Union[str, int, float]):
Copy link

Choose a reason for hiding this comment

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

It could also set a value for an existing param, no?

Suggested change
def add_param(self, param_name:str, value:Union[str, int, float]):
def set_param(self, param_name:str, value:Union[str, int, float]):

@@ -532,8 +534,8 @@ def search(self, query):
has_payload=query._with_payloads,
with_scores=query._with_scores)

def explain(self, query):
args, query_text = self._mk_query_args(query)
def explain(self, query, query_params: Dict[str, Union[str, int, float]] = None):
Copy link

Choose a reason for hiding this comment

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

Incompatible variable type: query_params is declared to have type Dict[str, Union[float, int, str]] but is used as type None.
(at-me in a reply with help or ignore)

@DvirDukhan DvirDukhan requested a review from rafie October 4, 2021 10:18
test/test.py Outdated
client.add_document('doc3', name='Carol')

params_dict = {"name1":"Alice", "name2":"Bob"}
q = Query("@name:($name1 | $name2 )").set_params_dict(params=params_dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to have something like:

params = {"name1":"Alice", "name2":"Bob"}
q = Query("@name:($name1 | $name2 )")
res1 = client.search(params=params)
res2 = client.search(params=other_params)

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have it, continue reading the test ;)

Comment on lines 514 to 515
if query_params is not None:
args+= Query.get_params_args(query_params)
Copy link

Choose a reason for hiding this comment

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

Do you want to use the member query._params if argument query_params is None?
(or even to merge both if both are not None, giving precedence to the argument over the member?)
Or do you want to get rid of the member? (it is only available for Query, not for AggregateRequest)

Copy link

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@sonarcloud
Copy link

sonarcloud bot commented Oct 14, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Nov 25, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@AvitalFineRedis
Copy link
Contributor

Should it also be supported in FT.PROFILE command?

@oshadmi
Copy link

oshadmi commented Dec 15, 2021

Should it also be supported in FT.PROFILE command?

Should work, but not yet tested.
Basically, if users have a query using Parameters, and they wish to profile it, it would be beneficial to profile the query with its Parameters using FT.PROFILE

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