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

added return_fields function, attempting to optionally limit fields r… #633

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sav-norem
Copy link
Contributor

…eturned by find. Will solve #568 when completed.

@sav-norem sav-norem marked this pull request as ready for review August 14, 2024 14:39
@@ -447,6 +448,11 @@ def __init__(
else:
self.sort_fields = []

if return_fields:
self.return_fields = self.validate_return_fields(return_fields)
Copy link
Member

Choose a reason for hiding this comment

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

since return_fields is being declared below as a method you can't perform this assignment (it makes the linter mad and probably leads to some invalid state)

@@ -504,8 +510,19 @@ def query(self):
if self._query.startswith("(") or self._query == "*"
else f"({self._query})"
) + f"=>[{self.knn}]"
if self.return_fields:
self._query += f" RETURN {','.join(self.return_fields)}"
Copy link
Member

Choose a reason for hiding this comment

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

The RETURN statement shouldn't be added to the query string, that's the filtration part of the query. Rather it needs to be passed to the list of args being passed to Redis. See the execute method inside of find_query to see what I mean, the RETURN, the number of fields being returned, and each individual field being returned need to be added as individual arguments to that list argument for it to be interpreted correctly.

Copy link

@DennyD17 DennyD17 Aug 22, 2024

Choose a reason for hiding this comment

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

sorry, what are you going to do with deserialization?
It seems like the query with RETURN returns data in different way... it was my problem

I mean https://github.com/redis/redis-om-python/blob/main/aredis_om/model/model.py#L1583

Choose a reason for hiding this comment

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

For me this works

    @staticmethod
    def create_return_part(fields: List[str] | Set[str]) -> List[str]:
        q = ["RETURN", "n"]

        for field in fields:
            q.extend([f"$.{field}", "AS", field])
        q[1] = len(q) - 2
        return q

Copy link
Member

Choose a reason for hiding this comment

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

IMO you'd return a dict[string,dict[string,string]] as that's essentially what the API responds with (technically it's an array of strings and arrays with an integer at it's head)

Choose a reason for hiding this comment

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

I'm talking about this behavior:
I have Redis (Search + Json)
When I run query (using redis-cli for example) without RETURN:
FT.SEARCH ':foo:index' '(@parent_id:[2 2])' LIMIT 1 1

I get the output like JSON document
3) 1) "$" 2) "{\"updated_at\":\"2024-09-13 12:14:55.877717\",\"id\":3,\"page_id\":3

And here are right data types (id and page id are integers)

But when I use RETURN
FT.SEARCH ':foo:index' '(@parent_id:[2 2])' LIMIT 0 1000 RETURN 6 $.id AS id, $.page_id AS page_id

I get the output like this

3) 1) "id,"
   2) "2"
   3) "page_id"
   4) "2"

So my question is how are you going to deserialize this results?

Choose a reason for hiding this comment

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

My fault guys,
Everything will be OK after validation
field.validate(value, values, loc=field.alias, cls=cls_)

I've turned it off, very expensive to run validations on read queries

Choose a reason for hiding this comment

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

But please check if it works with dict fields, I get error "value is not a valid dict" while processing '{"foo": "bar"}'

@py_test_mark_asyncio
async def test_return_specified_fields(members, m):
member1, member2, member3 = members
actual = await m.Member.find(
Copy link
Member

Choose a reason for hiding this comment

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

One issue to contend with here is that the model validation will not work correctly when running find, you'll need to make sure you return a dictionary or some sub-set of the model to prevent validation errors from being tossed by the find.

@DennyD17
Copy link

Please add also exclude keyword, it'll be convenient

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.

4 participants