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

Changed behavior of QueryResult #369

Closed
andreymal opened this Issue Jul 30, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@andreymal
Copy link

andreymal commented Jul 30, 2018

This change broke my website:

-class QueryResult(list):
+class QueryResult(object):

0.7.3:

>>> [7] + select(p for p in Person if p.age > 20)[:]
[7, Person[2], Person[3]]

0.7.5:

>>> [7] + select(p for p in Person if p.age > 20)[:]
TypeError: can only concatenate list (not "QueryResult") to list

If this is a bug, please fix this.

If this is a feature, please update the documentation and the changelog.

@kozlovsky kozlovsky self-assigned this Jul 30, 2018

@kozlovsky kozlovsky added the bug label Jul 30, 2018

@kozlovsky kozlovsky added this to the 0.7.6 milestone Jul 30, 2018

@andreymal

This comment has been minimized.

Copy link
Author

andreymal commented Jul 30, 2018

There's also another case: random.shuffle(query_result)

Not sure if it should work by design, but it works in 0.7.3 :)

(In general, class QueryResult is not documented, it would be good if it will mentioned in the documentation)

@kozlovsky

This comment has been minimized.

Copy link
Member

kozlovsky commented Jul 30, 2018

Hi, thanks for reporting. This is a bug, and we will fix it

@kozlovsky

This comment has been minimized.

Copy link
Member

kozlovsky commented Jul 30, 2018

QueryResult now is indeed not a subclass of list. The reason for this change is the following: in 0.7.3, when someone write:

for obj in query:
    ...

iter(query) immediately fetches query result.

But now it is possible to base a new query on a previous query:

query2 = select(x for x in query)

when query is used as a source of a new generator, Python immediately takes iter(query) even before the select function is called. In 0.7.3 it forces query execution. But we don't want to execute first query, it just a part of a final query.

The solution that we use in 0.7.4 is to make query result iterator lazy, and actually execute the query only when iterator.next is called the first time.

That is to say, the result of query[:] is still fetched immediately, while iter(query) is lazy now. Also, query.limit(x) now returns query instead of a query result.

And with this changes it is more clear not to base potentially lazy QueryResult on list, but to have a separate class for it.

To fix the problem that you described we need to add

For compatibility we can add QueryResult.__add__ which allows adding QueryResult to list. But I'm against adding mutable methods __iadd__, __setitem__, __delitem__, append, clear, extend, insert, pop, remove, reverse, sort, because mutating query result looks strange. So, the code like random.shuffle(query_result) will not work. You will need to replace it with:

query_result = list(query_result)
random.shuffle(query_result)

@kozlovsky kozlovsky closed this in b1cb57c Jul 31, 2018

@kozlovsky

This comment has been minimized.

Copy link
Member

kozlovsky commented Jul 31, 2018

Now QueryResult is not based on list, but expressions like

[7] + select(p for p in Person if p.age > 20)[:]

should work again.

Also, I added .sort(), .reverse() and .shuffle() methods directly to QueryResult

For more complex mutations of QueryResult you need to convert it to list first, either with list(query_result), or by calling query_result.to_list()

spl0k added a commit to spl0k/supysonic that referenced this issue Aug 5, 2018

kozlovsky added a commit that referenced this issue Aug 8, 2018

Pony ORM Release 0.7.6rc1 (2018-08-08)
# New features

* f-strings support in queries: select(f'{s.name} - {s.age}' for s in Student)
* #344: It is now possible to specify offset without limit: `query.limit(offset=10)`
* #371: Support of explicit casting of JSON expressions to `str`, `int` or `float`
* `@db.on_connect` decorator added

# Bugfixes

* Fix bulk delete bug introduced in 0.7.4
* #370 Fix memory leak introduced in 0.7.4
* Now exists() in query does not throw away condition in generator expression: `exists(s.gpa > 3 for s in Student)`
* #373: 0.7.4/0.7.5 breaks queries using the `in` operator to test membership of another query result
* #374: `auto=True` can be used with all PrimaryKey types, not only int
* #369: Make QueryResult looks like a list object again: add concatenation with lists, `.shuffle()` and `.to_list()` methods
* #355: Fix binary primary keys `PrimaryKey(buffer)` in Python2
* Interactive mode support for PyCharm console
* Fix wrong table aliases in complex queries
* Fix query optimization code for complex queries

kozlovsky added a commit that referenced this issue Aug 10, 2018

Pony ORM Release 0.7.6 (2018-08-10)
# Features since 0.7.5:

* f-strings support in queries: `select(f'{s.name} - {s.age}' for s in Student)`
* #344: It is now possible to specify offset without limit: `query.limit(offset=10)`
* #371: Support of explicit casting of JSON expressions to `str`, `int` or `float`
* `@db.on_connect` decorator added

# Bugfixes

* Fix bulk delete bug introduced in 0.7.4
* #370 Fix memory leak introduced in 0.7.4
* Now `exists()` in query does not throw away condition in generator expression: `exists(s.gpa > 3 for s in Student)`
* #373: 0.7.4/0.7.5 breaks queries using the `in` operator to test membership of another query result
* #374: `auto=True` can be used with all PrimaryKey types, not only `int`
* #369: Make QueryResult looks like a list object again: add concatenation with lists, `.shuffle()` and `.to_list()` methods
* #355: Fix binary primary keys `PrimaryKey(buffer)` in Python2
* Interactive mode support for PyCharm console
* Fix wrong table aliases in complex queries
* Fix query optimization code for complex queries
* Fix a bug with hybrid properties that use external functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.