reverse lookup prototype #599

sinisaos · 2022-08-25T18:53:01Z

Related to #378. Prototype for reverse foreign key lookup. Any suggestions and help are welcome. If I missed the point or it's not a good api design feel free to close PR.
Usage of reverse FK lookup:

from piccolo.table import Table
from piccolo.columns import Varchar, ForeignKey
from piccolo.columns.reference import LazyTableReference
from piccolo.columns.reverse_lookup import ReverseLookup

class Manager(Table):
    name = Varchar()
    artists = ReverseLookup(
        LazyTableReference("Artist", module_path=__name__),
    )

class Artist(Table):
    name = Varchar()
    manager = ForeignKey(Manager)

rows = Manager.select(
        Manager.name, Manager.artists(Artist.id, Artist.name, table=Manager) # or as list Manager.artists(Artist.name, as_list=True, table=Manager)
    ).run_sync()

codecov-commenter · 2022-08-26T05:14:18Z

Codecov Report

Merging #599 (a7405b7) into master (ff26a88) will decrease coverage by 0.02%.
The diff coverage is 88.79%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
- Coverage   91.35%   91.33%   -0.02%     
==========================================
  Files         108      109       +1     
  Lines        7517     7629     +112     
==========================================
+ Hits         6867     6968     +101     
- Misses        650      661      +11

Impacted Files	Coverage Δ
piccolo/columns/m2m.py	`91.19% <ø> (+1.13%)`	⬆️
piccolo/query/methods/select.py	`96.63% <87.80%> (-1.27%)`	⬇️
piccolo/columns/reverse_lookup.py	`88.73% <88.73%> (ø)`
piccolo/table.py	`96.40% <100.00%> (+0.01%)`	⬆️

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

dantownsend · 2022-08-26T12:26:25Z

Thanks for this! It looks very promising.

sinisaos · 2022-08-26T13:58:34Z

Thanks. I used a lot of code (with some modifications) from M2M fields. I tried to use the best names of variables and methods. The main obstacle is that the user would have to enter reverse_lookup_name. I will try to make that optional (if table has only one FK default index value for foreign_key_columns will be 0), so that the user has to specify reverse_lookup_name only if the table has multiple FK so ReverseLookup knows which column to use. If you have any ideas, any help is welcome.
UPDATE:
I removed the reverse_lookup_name and added a table parameter to the ReverseLookupSelect class. With that, Piccolo can identify which table to use for reverse lookup if the table has multiple FKs. I think that is a better option.

powellnorma · 2022-12-16T15:02:14Z

Just wanted to note that there is interest in this functionality. Is there anything specific holding this back (from being merged)?

Also I like how in peewee one can just set the backref parameter on the ForeignKeyField, see here
Could we have a similar API in piccolo?

powellnorma · 2022-12-16T15:45:06Z

piccolo/columns/reverse_lookup.py

+            i._meta.name
+            for i in reverse_lookup_table._meta.foreign_key_columns
+        ]
+        return fk_columns.index(self.table._meta.tablename)


It looks like we compare column names with table names here. They don't have to match. I think instead of

Manager.artists(table=Manager)

it would be better to be able to specify:

Manager.artists(column="manager")

Since the column that is used for the reverse lookup doesn't change(?), I think it would be best if we could also specify it in the ReverseLookup constructor.

@powellnorma Thanks for your interest and review. It's been a while since I wrote this code, but if I remeber it has to be a table because we refer to the foreign key of parent table later in the code. We need this function to find the foreign key column index if there are multiple foreign keys per single table.

dantownsend · 2022-12-16T15:50:01Z

Just wanted to note that there is interest in this functionality. Is there anything specific holding this back (from being merged)?

I haven't had chance to review it yet - but I agree that it's a feature we need.

powellnorma · 2022-12-16T16:24:01Z

piccolo/columns/reverse_lookup.py

+        reverse_lookup_fk_table_name = (
+            reverse_lookup_table._meta.foreign_key_columns[
+                self.foreign_key_columns_index
+            ]._meta.name


I think instead of _meta.name it should be _meta.table._meta.tablename. Otherwise reverse_lookup_fk_table_name is a column name

powellnorma · 2022-12-16T16:26:15Z

piccolo/columns/reverse_lookup.py

+                        SELECT
+                            "{reverse_lookup_table_name}"."{column_name}"
+                        FROM {reverse_select}
+                    ) AS "{reverse_lookup_table_name}s"


I think we should use AS "{original_column_name}s". The table must give us that name. See m2m.py:

piccolo/piccolo/columns/m2m.py

Line 93 in 14012e5

) AS "{m2m_relationship_name}"

sinisaos · 2022-12-16T16:45:42Z

@dantownsend It would be great if you could find the time to review this. @powellnorma This code works and passes all tests. If you think that things should be changed, it would be best after Daniel's review. Thank you both.

powellnorma · 2022-12-16T19:50:20Z

piccolo/columns/reverse_lookup.py

+        reverse_select = f"""
+            "{reverse_lookup_table_name}"
+            JOIN "{reverse_lookup_fk_table_name}" "inner_{reverse_lookup_fk_table_name}" ON (
+                "{reverse_lookup_table_name}"."{reverse_lookup_fk}"
+                = "inner_{reverse_lookup_fk_table_name}"."{reverse_lookup_pk}"
+            ) WHERE "{reverse_lookup_table_name}"."{reverse_lookup_fk}"
+                = "{reverse_lookup_fk_table_name}"."{reverse_lookup_pk}"
+        """  # noqa: E501


Why do we need this JOIN here? Isn't the following enough?

reverse_select = f""" "{reverse_lookup_table_name}" WHERE "{reverse_lookup_table_name}"."{reverse_lookup_fk}" = "{reverse_lookup_fk_table_name}"."{reverse_lookup_pk}" """

powellnorma · 2022-12-16T19:56:21Z

This code works and passes all tests

I haven't looked, but I guess the tests always use a ForeignKey named the same as the referenced Table. As e.g. here:

class Manager(Table):
    name = Varchar()

class Artist(Table):
    name = Varchar()
    manager = ForeignKey(Manager)

But if this is not true, as e.g. here:

class MainManager(Table):
    name = Varchar()

class Artist(Table):
    name = Varchar()
    manager = ForeignKey(MainManager)

the sections I pointed lead to errors / wrong queries

powellnorma · 2022-12-16T20:03:32Z

When I query an M2M field of the referenced table, I get:

AttributeError: type object 'M2MSelect' has no attribute 'value_type'

Make it work when the ForeignKey is named differently than the referenced Table.

powellnorma · 2022-12-16T20:39:39Z

I have fixed the parts that I reviewed on my branch reverse_lookup, see: https://github.com/powellnorma/piccolo/commits/reverse_lookup

However querying an M2M field (of the referenced table) seems to be rather slow at the moment. When I set self.serialisation_safe = False in the case of the above mentioned AttributeError, it seems that all m2m joining keys are being used as arguments to a second query. This is overall (in my case) slower than using a for loop in python (which is located in a transaction) instead of the ReverseLookup.
@dantownsend what could we do to make this (querying m2m of reverse-referenced table) faster?

sinisaos · 2022-12-16T22:10:56Z

But if this is not true, as e.g. here:
class MainManager(Table):
    name = Varchar()

class Artist(Table):
    name = Varchar()
    manager = ForeignKey(Manager)
the sections I pointed lead to errors / wrong queries

I'm sorry, but I don't understand. How can we refer to a table that doesn't exist. Shouldn't it be like this

class MainManager(Table):
    name = Varchar()

class Artist(Table):
    name = Varchar()
    manager = ForeignKey(MainManager)

I want to try your branch. In my reverse lookup attempt I use LazyTableReference like this

class Manager(Table):
    name = Varchar()
    bands = ReverseLookup(
        LazyTableReference("Band", module_path=__name__),
    )


class Band(Table):
    name = Varchar()
    manager = ForeignKey(Manager)

# query for all managers bands
query = await Manager.select(
        Manager.name, Manager.bands(Band.name, as_list=True, table=Manager)
    )

Can you show me how to write a table schema and an example query because right now I get TypeError: __init__() missing 1 required positional argument: 'reverse_fk'. Thanks in advance

powellnorma · 2022-12-16T23:43:23Z

I'm sorry, but I don't understand. How can we refer to a table that doesn't exist.

The example I gave was wrong, I corrected it. What I meant was a situation in which the column name (of the foreign key) has a different name than the table that it refers to

Can you show me how to write a table schema and an example query because right now I get TypeError: init() missing 1 required positional argument: 'reverse_fk'.

Like this:

class MainManager(Table):
    name = Varchar()
    artists = ReverseLookup(LazyTableReference("Artist", module_path=__name__), "manager")

class Artist(Table):
    name = Varchar()
    manager = ForeignKey(MainManager)

# select:
await MainManager.select(MainManager.artists(Artist.name))

sinisaos · 2022-12-17T08:16:14Z

The example I gave was wrong, I corrected it. What I meant was a situation in which the column name (of the foreign key) has a different name than the table that it refers to

You are right, it doesn't work if the column is different from the table name. I tried to use foreign_key_columns_index so that if the table has multiple fk columns we know the index of those columns, but it obviously doesn't work well.

class MainManager(Table):
    name = Varchar()
    artists = ReverseLookup(LazyTableReference("Artist", module_path=__name__), "manager")

class Artist(Table):
    name = Varchar()
    manager = ForeignKey(MainManager)

# select:
await MainManager.select(MainManager.artists(Artist.name))

This also doesn't work. Raise ValueError: _name isn't defined - the Table Metaclass should set it. on line 148 name property.

powellnorma · 2022-12-17T11:28:37Z

This also doesn't work. Raise ValueError: _name isn't defined - the Table Metaclass should set it. on line 148 name property.

Hm, for me that example works. I (already) made changes so that _name should (indeed) be set from the Table Metaclass here.
Have you made sure that you checked out my branch correctly? Maybe only parts of my changes got applied?

sinisaos · 2022-12-17T15:08:30Z

Sorry, that was my mistake. I tried everything from scratch because I probably messed something up and you are right, the tests from my PR pass with your code.
The only thing I've noticed is that if the column name is plural the tests fail, but singular name pass. I tried to strip column name if it ends with s and it works in Sqlite but not in Postgres.
I have to go now and will be gone for 2 days, but you should continue with this because your version is much better and easier to understand and it works. I'll be happy to delete my PR so you can make a new one. I'll try to help as much as I can.

powellnorma · 2022-12-18T13:40:36Z

@sinisaos I created a PR on your fork/branch. If you accept it, my changes should be included in this PR.

The only thing I've noticed is that if the column name is plural the tests fail

Do you have an example that demonstrates this problem? How can I reproduce this? Thank you

powellnorma · 2022-12-18T23:32:03Z

The only thing I've noticed is that if the column name is plural the tests fail

Does it still occur after this commit? sinisaos@d6c6453

Reverse lookup: some fixes for PR

sinisaos · 2022-12-19T20:37:49Z

@powellnorma I have added your changes. Thanks again for your help.

lqmanh · 2023-05-02T17:04:37Z

Hi, just wonder what's the status quo of this PR, as it's been around for a long time. Are there any blockers left? I'd really love to see this feature getting merged.

Thanks you guys in advance.

dantownsend · 2023-05-02T20:24:17Z

@lqmanh Will try and get this merged in soon. It just needs reviewing in detail to make sure we're happy with the API 👍

powellnorma · 2023-08-18T06:27:35Z

One thing this is currently missing is the ability to use .order_by on the ReverseLookupSelect. Similarly with .where etc. @dantownsend Do you think this should be supported? And if so, do you see a way in which we could reuse the existing implementations for these methods?

erhuabushuo · 2023-10-08T04:27:20Z

Wow, I'm waiting for this PR.

py-radicz · 2023-10-26T08:05:58Z

it would be cool to have reverse lookup in the api, cant wait for it

dantownsend · 2023-10-26T17:19:20Z

OK, seems like a lot of people want this. Getting Piccolo v1 out has taken most of my attention. Will take another look at this once I've released Python 3.12 support. Bear with me, I know this PR has been around for a while 🙏

kayprogrammer · 2024-01-27T01:38:16Z

You guys are doing well.
Still waiting for this PR, including composite unique constraints and check constraints.

sinisaos added 2 commits August 25, 2022 20:41

reverse lookup prototype

30acf3c

docstring fix

5940304

sinisaos added 4 commits August 28, 2022 08:36

remove reverse_lookup_name

c74e89e

update docstring

2839931

Merge branch 'piccolo-orm:master' into reverse_lookup

dfc1bb0

Merge dfc1bb0 into 45dbb8b

292186b

dantownsend mentioned this pull request Oct 17, 2022

One-to-many relations #220

Closed

powellnorma reviewed Dec 16, 2022

View reviewed changes

powellnorma added 2 commits December 16, 2022 21:29

Merge branch 'master' into reverse_lookup

8f47af2

reverse lookup: some fixes

f98e0b3

Make it work when the ForeignKey is named differently than the referenced Table.

reverse_lookup.py: update docstrings

9a5ba3e

powellnorma mentioned this pull request Dec 18, 2022

M2M and all_columns #728

Closed

reverse lookup: remove excess 's'

d6c6453

sinisaos added 3 commits December 19, 2022 20:02

Merge pull request #1 from powellnorma/reverse_lookup

c4f2777

Reverse lookup: some fixes for PR

Merge branch 'piccolo-orm:master' into reverse_lookup

5f5221b

fix tests and linter errors

b3a0e27

dantownsend mentioned this pull request Dec 23, 2022

[Question] Need help with table joins #735

Open

powellnorma mentioned this pull request Feb 21, 2023

List aggregate #766

Open

Merge branch 'master' into reverse_lookup

a7405b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reverse lookup prototype #599

reverse lookup prototype #599

sinisaos commented Aug 25, 2022 •

edited

codecov-commenter commented Aug 26, 2022 •

edited

dantownsend commented Aug 26, 2022

sinisaos commented Aug 26, 2022 •

edited

powellnorma commented Dec 16, 2022

powellnorma Dec 16, 2022 •

edited

sinisaos Dec 16, 2022

dantownsend commented Dec 16, 2022

powellnorma Dec 16, 2022

powellnorma Dec 16, 2022

sinisaos commented Dec 16, 2022

powellnorma Dec 16, 2022

powellnorma commented Dec 16, 2022 •

edited

powellnorma commented Dec 16, 2022

powellnorma commented Dec 16, 2022 •

edited

sinisaos commented Dec 16, 2022

powellnorma commented Dec 16, 2022 •

edited

sinisaos commented Dec 17, 2022

powellnorma commented Dec 17, 2022

sinisaos commented Dec 17, 2022

powellnorma commented Dec 18, 2022

powellnorma commented Dec 18, 2022

sinisaos commented Dec 19, 2022

lqmanh commented May 2, 2023

dantownsend commented May 2, 2023

powellnorma commented Aug 18, 2023

erhuabushuo commented Oct 8, 2023

py-radicz commented Oct 26, 2023

dantownsend commented Oct 26, 2023 •

edited

kayprogrammer commented Jan 27, 2024

reverse lookup prototype #599

Are you sure you want to change the base?

reverse lookup prototype #599

Conversation

sinisaos commented Aug 25, 2022 • edited

codecov-commenter commented Aug 26, 2022 • edited

Codecov Report

dantownsend commented Aug 26, 2022

sinisaos commented Aug 26, 2022 • edited

powellnorma commented Dec 16, 2022

powellnorma Dec 16, 2022 • edited

Choose a reason for hiding this comment

sinisaos Dec 16, 2022

Choose a reason for hiding this comment

dantownsend commented Dec 16, 2022

powellnorma Dec 16, 2022

Choose a reason for hiding this comment

powellnorma Dec 16, 2022

Choose a reason for hiding this comment

sinisaos commented Dec 16, 2022

powellnorma Dec 16, 2022

Choose a reason for hiding this comment

powellnorma commented Dec 16, 2022 • edited

powellnorma commented Dec 16, 2022

powellnorma commented Dec 16, 2022 • edited

sinisaos commented Dec 16, 2022

powellnorma commented Dec 16, 2022 • edited

sinisaos commented Dec 17, 2022

powellnorma commented Dec 17, 2022

sinisaos commented Dec 17, 2022

powellnorma commented Dec 18, 2022

powellnorma commented Dec 18, 2022

sinisaos commented Dec 19, 2022

lqmanh commented May 2, 2023

dantownsend commented May 2, 2023

powellnorma commented Aug 18, 2023

erhuabushuo commented Oct 8, 2023

py-radicz commented Oct 26, 2023

dantownsend commented Oct 26, 2023 • edited

kayprogrammer commented Jan 27, 2024

sinisaos commented Aug 25, 2022 •

edited

codecov-commenter commented Aug 26, 2022 •

edited

sinisaos commented Aug 26, 2022 •

edited

powellnorma Dec 16, 2022 •

edited

powellnorma commented Dec 16, 2022 •

edited

powellnorma commented Dec 16, 2022 •

edited

powellnorma commented Dec 16, 2022 •

edited

dantownsend commented Oct 26, 2023 •

edited