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

fix: add _use/_force_indexes into __copy__ with shallow copy #631

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

elisong
Copy link
Contributor

@elisong elisong commented Aug 29, 2021

By #334, #470 , pypika got force_indexuse_index feature, but forgot to add "_force_indexes", "_use_indexes" into QueryBuilder.__copy__() with shallow copy, thus they both become mutable. For examples

from pypika import Query

query = Query.from_('customers').select('id', 'fname')
q1 = query.use_index('idx1')
q2 = query
q3 = query.use_index('idx2')

if __name__ == "__main__":
    print(q1.get_sql())
    # SELECT "id","fname" FROM "customers" USE INDEX ("idx1","idx2")
    print(q2.get_sql())
    # SELECT "id","fname" FROM "customers" USE INDEX ("idx1","idx2")
    print(q3.get_sql())
    # SELECT "id","fname" FROM "customers" USE INDEX ("idx1","idx2")

Copy link
Contributor

@x8lucas8x x8lucas8x left a comment

Choose a reason for hiding this comment

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

@elisong that looks good, but can you please add tests to ImmutabilityTests, so that this can be tested. So far we only have join tests there (i.e. test_queries_after_join). If you could add test cases for other things that are being copied as well (see copy), such as selects, groupbys, and so on, that would be great (e.g. test_queries_after_groupby, test_queries_after_select).

@x8lucas8x x8lucas8x closed this Aug 31, 2021
@x8lucas8x x8lucas8x reopened this Aug 31, 2021
@coveralls
Copy link

coveralls commented Aug 31, 2021

Coverage Status

Coverage increased (+0.01%) to 98.419% when pulling b9aa50e on elisong:add-use/force-indexes-shallow-copy into 46c90d1 on kayak:master.

@elisong
Copy link
Contributor Author

elisong commented Sep 2, 2021

@x8lucas8x Fine, I added some tests.

@x8lucas8x
Copy link
Contributor

@elisong thank you. It looks great now.

@x8lucas8x x8lucas8x merged commit a7b01da into kayak:master Sep 3, 2021
@elisong elisong deleted the add-use/force-indexes-shallow-copy branch September 3, 2021 14:24
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