Skip to content

fix optimization for bulk updates #224

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

Closed
wants to merge 6 commits into from
Closed

fix optimization for bulk updates #224

wants to merge 6 commits into from

Conversation

mxm
Copy link
Contributor

@mxm mxm commented May 17, 2017

  • don't optimize if parameters are not supplied yet
  • check the key type before binding parameters

- don't optimize if parameters are not supplied yet
- check the key type before binding parameters
Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

Could we also add a integration test for the bulk update?

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

Changes.txt entry is missing

@mxm
Copy link
Contributor Author

mxm commented May 17, 2017

Thanks for the comments @mfussenegger. I've added an integration test and updated the CHANGES file.

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

Please check the travis failures

CHANGES.txt Outdated
=================

- Fix bulk updates which were broken due to query rewrites.
- Fix typo to DB API in README
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to mention the README fix - only end-user relevant changes should end up in the changes.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Thanks.

@jeffnappi
Copy link

Thanks for jumping in on this @mxm - I was just about to jump in and fix it myself :)

Looks like the Travis Tests are failing simply due to the missing newline in update_test.py

Testing now to see if this resolves my issue - appears like it should.

@mxm
Copy link
Contributor Author

mxm commented May 18, 2017

Thanks @jeffnappi. I fixed the missing newline. Let's wait for the tests to pass.

@mxm
Copy link
Contributor Author

mxm commented May 18, 2017

One of my new tests is failing with SQLAlchemy 1.0.16 but passes with 1.1.4. Let's see.

@mxm
Copy link
Contributor Author

mxm commented May 18, 2017

@mfussenegger Is this good to merge?

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

lgtm

@mxm mxm closed this in d6d5386 May 18, 2017
@mxm mxm deleted the mxm/223 branch May 18, 2017 13:58
seut pushed a commit that referenced this pull request Jun 20, 2017
- don't optimize if parameters are not supplied yet
- check the key type before binding parameters
- add test cases for bulk updates

This closes #224
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.

3 participants