🐛 FIX distinct queries #5654
🐛 FIX distinct queries #5654
Conversation
|
Cheers @chrisjsewell , seems the new regression test passes, just a minor thing with |
|
Just cycling home from football, then I'll be finishing it 😉 |
|
see the updated initial comment @sphuber |
| # we discard the first item of the result row, | ||
| # which was what the query was initialised with and not one of the requested projection (see self._build) | ||
| result = result[1:] | ||
|
|
||
| if len(result) != self._requested_projections: | ||
| raise AssertionError( | ||
| f'length of query result ({len(result)}) does not match ' | ||
| f'the number of specified projections ({self._requested_projections})' | ||
| ) | ||
| # SQLA will return a Row, if only certain columns are requested, | ||
| # or a database model if that is requested | ||
| if not isinstance(result, Row): | ||
| result = (result,) |
There was a problem hiding this comment.
This is the only part I was a little unsure of; since there can now be only a single projection (as opposed to >=2 before), the return type can be different, whether you are returning a whole row e.g. Node or a single column e.g Node.id.
Without this, the tests were failing, but with it, they appear fine 🤷
There was a problem hiding this comment.
Thanks @chrisjsewell . Looks good, just have some really minor comments. Would also just suggest to describe the fix well in the commit message when merging, because with all the additional refactoring, it is not that easy to see what the actual fix was.
| """A method that returns an valid SQLAlchemy expression. | ||
|
|
||
| :param operator: The operator provided by the user ('==', '>', ...) | ||
| :param value: The value to compare with, e.g. (5.0, 'foo', ['a','b']) | ||
| :param column: an instance of sqlalchemy.orm.attributes.InstrumentedAttribute or | ||
|
|
||
| """ |
There was a problem hiding this comment.
Any reason to remove the docstring?
There was a problem hiding this comment.
It was just a copy of the parent docstring, so then I think it is just general practice to have it "inherit" that?
This commit principally fixes issues with distinct queries. Since refactoring for sqlalchemy 1.4, the query has been initialised with a starting "dummy" projection from which to join. ```python query = session.query(starting_table.id) ``` The returned projection was then removed. This is problematic, though, when requesting distinct result rows, because the dummy projection is also used to calculate uniqueness. This has now been changed to use the `select_from` method: https://docs.sqlalchemy.org/en/14/orm/query.html#sqlalchemy.orm.Query.select_from, such that now we can initialise without any projection. ```python query = session.query().select_from(starting_table) ``` The backend QueryBuilder code is also refactored, principally to make the `SqlaQueryBuilder._build` logic more understandable. Cherry-pick: 9fa2d88
Fixes #5535
The key change in this PR is changing (approximately):
to:
In this way, we no longer need to deal with the "dummy" projection,
which was causing issues with the identification of distinct items.
(see also: https://docs.sqlalchemy.org/en/14/orm/query.html#sqlalchemy.orm.Query.select_from)
I also took the opportunity to do some refactoring,
principally to make the
SqlaQueryBuilder._buildcode more understandable.