crate / sqlalchemy-cratedb

SQLAlchemy dialect for CrateDB.
https://cratedb.com/docs/sqlalchemy-cratedb/
Apache License 2.0
3 stars 2 forks source link

Updates Across All Records Failing #125

Closed jeffnappi closed 7 years ago

jeffnappi commented 7 years ago

In the process of updating our codebase from SQLAlchemy==0.9.2 + crate==0.16.1 to SQLAlchemy==1.1.9 + crate==0.19.2, I immediately encountered this error in our test suite:

  File "site-packages/sqlalchemy/engine/base.py", line 1015, in _execute_clauseelement
    fn(self, elem, multiparams, params)
  File "site-packages/crate/client/sqlalchemy/compiler.py", line 76, in crate_before_execute
    return rewrite_update(clauseelement, multiparams, params)
  File "site-packages/crate/client/sqlalchemy/compiler.py", line 67, in rewrite_update
    clause = clauseelement.values(newmultiparams[0])
IndexError: list index out of range

This test simply exercises the ability to update all documents via the SQLAlchemy ORM:

        session.query(Post).update({
            Post.social_scheduled_at.name: datetime.utcnow()
        })

Such syntax is clearly documented as correct in the SQLAlchemy Docs: http://docs.sqlalchemy.org/en/rel_1_1/orm/query.html#sqlalchemy.orm.query.Query.update

I would be glad to help work through this issue - but might need some help following what compiler#rewrite_update is trying to accomplish.

mfussenegger commented 7 years ago

Thanks for reporting, we'll take a look at this.

The rewrite_update method is there to narrow the update statement. If you only updated a single property on an object, it will only update that instead of the whole object: instead of update t set x = ? it will execute update t set x['y']['z'] = ? if possible. (This has the advantage that we can resolve conflicts on the server side. For example if one client issued set x['a'] = 10 and another set x['b'] = 20 a conflict can be resolved because we know that different properties are updated. If it was issued like set x = {"a": 10, "b": 10} and set x = {"a": 5, "b": 20}, the second update would overwrite the a value.

mxm commented 7 years ago

Hi @jeffnappi. I took a look at the problem and opened a PR https://github.com/crate/crate-python/pull/224. Your code should run afterwards. If not, maybe you could provide your Post model?

jeffnappi commented 7 years ago

@mxm Looks like it works :)

mxm commented 7 years ago

Merged the PR. Should be fixed. If not, feel free to re-open!

mxm commented 7 years ago

This is also in 0.19.3 now.