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

Gel sqla #536

Merged
merged 4 commits into from
Dec 7, 2024
Merged

Gel sqla #536

merged 4 commits into from
Dec 7, 2024

Conversation

vpetrovykh
Copy link
Member

@vpetrovykh vpetrovykh commented Nov 14, 2024

More tests coming...

@vpetrovykh vpetrovykh self-assigned this Nov 14, 2024
@vpetrovykh vpetrovykh force-pushed the gel-sqla branch 2 times, most recently from 0885a10 to fd5217c Compare November 21, 2024 12:20
@msullivan
Copy link
Member

Could you post some generated code?

edgedb/_testbase.py Outdated Show resolved Hide resolved
@vpetrovykh vpetrovykh force-pushed the gel-sqla branch 3 times, most recently from 8c0f82e to aa57a94 Compare November 27, 2024 19:00
@vpetrovykh vpetrovykh marked this pull request as ready for review November 27, 2024 19:00
@vpetrovykh vpetrovykh force-pushed the gel-sqla branch 3 times, most recently from 6aa78ae to 61c9c75 Compare December 4, 2024 08:08
Create separate intermediate objects to represent links with link
properties (because ORMs tend to view them as such).
Don't allow crazy names (not usual type of identifiers).

Multi properties behave more like plain multi links than anything else
(because they have a separate table with the property value in it),
so they should be reflected like that establishing a relationship.

Reflect Gel modules into Python modules.

Add tests that setup a Gel database and then generate the SQLAlchemy
models from it. The individual tests use a SQLAlchemy session to access
the database using postgres protocol.
Comment on lines +213 to +226
for bklinks in bk.values():
if len(bklinks) > 1:
# We have a collision, so each backlink in it must now be
# disambiguated.
for link in bklinks:
origsrc = get_sql_name(link['target']['name'])
lname = link['name']
link['name'] = f'{lname}_from_{origsrc}'
# Also update the original source of the link with the
# special backlink name.
source = type_map[link['target']['name']]
fwname = lname.replace('backlink_via_', '', 1)
link['fwname'] = fwname
source['backlink_renames'][fwname] = link['name']
Copy link
Member

Choose a reason for hiding this comment

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

Could we always generate the disambiguated backlink, and then only generate the short name when it is unambiguous, or does that not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

We mostly expect not too many collisions so I tend to treat that as a the default. Backlink collisions appear when everything is linked to everything, which is not necessarily a common schema since that creates a lot of links to maintain.
Also, detecting unambiguous backlinks is a little more annoying that the other way around. Collisions are shown by identical names, but unambiguous long names would have to be checked by looking up their corresponding forward link name, seems more roundabout.

@msullivan msullivan closed this Dec 4, 2024
@msullivan msullivan reopened this Dec 4, 2024
# have psycopg2 installed, as long as we run this in the test
# environments that have this, it is fine since we're not expecting
# different functionality based on flavours of psycopg2.
if importlib.util.find_spec("psycopg2") is None:
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be cleaner to do

try:
    import psychopg2
except ImportError:
    # handle it

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to avoid the import since I don't even need anything from psycopg2 directly, not using it anywhere myself. Didn't want to leave the impression that I needed the import myself and if it's not used, maybe it's OK to "remove this dependency check" making it look like an outdated comment or dead code. importlib seemed like a more overt and unambiguous check.

@msullivan msullivan closed this Dec 4, 2024
@msullivan msullivan reopened this Dec 4, 2024
@msullivan msullivan merged commit 645fce0 into master Dec 7, 2024
61 checks passed
@msullivan msullivan deleted the gel-sqla branch December 7, 2024 00:33
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.

2 participants