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

New release for SQLAlchemy >= 2.0.22 #729

Closed
hashworks opened this issue Feb 27, 2024 · 19 comments
Closed

New release for SQLAlchemy >= 2.0.22 #729

hashworks opened this issue Feb 27, 2024 · 19 comments

Comments

@hashworks
Copy link

#725 added some fixes for SQLAlchemy >= 2.0.22, but there is no new version yet. Could you release one? This is blocking a python-sqlalchemy update on Arch Linux.

@josecsotomorales
Copy link
Collaborator

We are on the same boat here, I see some tests failing but unrelated to the PR merged.

@josecsotomorales
Copy link
Collaborator

It's failing since this PR

@marksteward
Copy link
Collaborator

This is also breaking sqlalchemy-continuum. For now I've pinned SQLAlchemy but I'm looking at removing the dependency.

@kvesteri
Copy link
Owner

kvesteri commented Mar 14, 2024

@josecsotomorales I added you as a collaborator, if you send me your PyPI username I can you add there as a maintainer as well.

@josecsotomorales
Copy link
Collaborator

Thank you @kvesteri, my PyPi username is josecsoto91

@josecsotomorales
Copy link
Collaborator

josecsotomorales commented Mar 14, 2024

FYI working on getting master to pass tests in this PR

@josecsotomorales
Copy link
Collaborator

Can I get some help reviewing this PR? There's an assertion that I had to comment, not clear why it's failing.

@marksteward
Copy link
Collaborator

I don't see how the changes you've made to aggregates.py can be right. What have you done to debug it?

@josecsotomorales
Copy link
Collaborator

I tested with setting up echo to true and debug the generated queries, with the previous local condition it was generating a cartesian product in the UPDATE ... FROM query from the association table

@josecsotomorales
Copy link
Collaborator

@kvesteri could you please add @marksteward as a contributor as well?

@marksteward
Copy link
Collaborator

marksteward commented Mar 21, 2024

The test first fails on 2.0.13, presumably due to sqlalchemy/sqlalchemy@4a62625. The query is:

UPDATE "user" SET group_count=(SELECT count(:count_2) AS count_1 
FROM "group" JOIN user_group AS user_group_1 ON "group".id = user_group_1.group_id 
WHERE "user".id = user_group_1.user_id) FROM "group" WHERE "group".id IN (__[POSTCOMPILE_id_1])'

Which looks wrong to me, it should be filtering on the user table only. The problem is it's explicitly pulling out the second entry in the relationship when a secondary is set. This code was introduced in 2014 (3ca0e9a), and trying with 0.9 (October 2014), local_remote_pairs is the same structure as now (i.e. local-to-secondary to begin, secondary-to-remote following):

[(Column('id', Integer(), table=<user>, primary_key=True, nullable=False),
  Column('user_id', Integer(), ForeignKey('user.id'), table=<user_group>)),
 (Column('id', Integer(), table=<group>, primary_key=True, nullable=False),
  Column('group_id', Integer(), ForeignKey('group.id'), table=<user_group>))]

The whole of local_condition is pretty odd, and doesn't look like it would support composite foreign keys. I'd be tempted to just skip the test or drop this code entirely.

@josecsotomorales
Copy link
Collaborator

Doh! That makes sense, I will rollback and just skip the test for now.

@josecsotomorales
Copy link
Collaborator

Merged to master the config fix by skipping the two problematic tests, from comments in the SA 2.0.22 issue seems like we need to refactor this PR

@josecsotomorales
Copy link
Collaborator

@marksteward created a PR for SA >= 2.0.22, can you review it? I think this approach will work for older versions as well.

@marksteward
Copy link
Collaborator

lgtm!

@josecsotomorales
Copy link
Collaborator

@kvesteri could you please send me another invitation to PyPI? Sorry I missed the previous one.

@kvesteri
Copy link
Owner

@josecsotomorales I invited you again and PyPi and invited @marksteward as collaborator here in github for this project and also as a maintainer for this package in PyPi

@josecsotomorales
Copy link
Collaborator

Thanks @kvesteri!! Released 0.41.2 on PyPI

@josecsotomorales
Copy link
Collaborator

Closing this issue now, thanks all 🚀

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

No branches or pull requests

4 participants