Fix fetch order by bug when using DISTINCT #963
Conversation
jverswijver
changed the title
jverswijver
changed the title
There was a problem hiding this comment.
Thanks for the PR @jverswijver! Primarily just have some questions about other similar usage of DISTINCT in source.
datajoint/expression.py
Outdated
| distinct = self.heading.names == self.primary_key | ||
| return 'SELECT {distinct}{fields} FROM {from_}{where}'.format( | ||
| distinct="DISTINCT " if distinct else "", | ||
| return 'SELECT {fields} FROM {from_}{where}'.format( |
There was a problem hiding this comment.
Can you also explore all usage of DISTINCT. Noticed that there is another similar usage in the Aggregation class. Did you mean to remove its use here but maintain its use there?
There was a problem hiding this comment.
I looked through it a bit and saw that it also uses DISTINCT, but it might be best to just leave it until a user reports a specific issue that is related to the aggregation usage of distinct. It would be a bad idea to remove it without a specific reason/issue.
docs-parts/intro/Releases_lang1.rst
Outdated
| @@ -4,6 +4,7 @@ | |||
| * Bugfix - Replace use of numpy aliases of built-in types with built-in type. (#938) PR #939 | |||
| * Bugfix - `ExternalTable.delete` should not remove row on error (#953) PR #956 | |||
| * Bugfix - Fix error handling of remove_object function in `s3.py` (#952) PR #955 | |||
| * Bugfix - Fix regression issue with DISCINCT clause and GROUP_BY (#914) PR #963 | |||
There was a problem hiding this comment.
| * Bugfix - Fix regression issue with DISCINCT clause and GROUP_BY (#914) PR #963 | |
| * Bugfix - Fix regression issue with DISTINCT clause and GROUP_BY (#914) PR #963 |
CHANGELOG.md
Outdated
| @@ -5,6 +5,7 @@ | |||
| * Bugfix - Replace use of numpy aliases of built-in types with built-in type. (#938) PR #939 | |||
| * Bugfix - `ExternalTable.delete` should not remove row on error (#953) PR #956 | |||
| * Bugfix - Fix error handling of remove_object function in `s3.py` (#952) PR #955 | |||
| * Bugfix - Fix regression issue with DISCINCT clause and GROUP_BY (#914) PR #963 | |||
There was a problem hiding this comment.
| * Bugfix - Fix regression issue with DISCINCT clause and GROUP_BY (#914) PR #963 | |
| * Bugfix - Fix regression issue with DISTINCT clause and GROUP_BY (#914) PR #963 |
tests/test_fetch.py
Outdated
|
|
||
| try: | ||
| Parent().fetch('KEY', order_by='name') | ||
| assert True |
tests/test_fetch.py
Outdated
| def test_fetch_group_by(self): | ||
| # https://github.com/datajoint/datajoint-python/issues/914 | ||
|
|
||
| try: |
There was a problem hiding this comment.
This try ... except does not appear to be necessary. Just letting the exception raise will have the same effect.
|
dj.U('attr') & qwhere Another case is unions. q + rIf we don't have test cases that require |
|
A few updates:
|
There was a problem hiding this comment.
@jverswijver Nice work in determining the root cause for this one! 💪
Only providing some minor feedback here.
Co-authored-by: Raphael Guzman <[email protected]>
tests/test_fetch.py
Outdated
| feched_result = result.fetch(as_dict=True) | ||
|
|
||
| # Cleanup table | ||
| Stimulus.delete() | ||
|
|
||
| # Test to see if the repeated row was removed in the results | ||
| assert feched_result == expected_result |
There was a problem hiding this comment.
Fetched results do not come in any particular order, so need to order_by.
| feched_result = result.fetch(as_dict=True) | |
| # Cleanup table | |
| Stimulus.delete() | |
| # Test to see if the repeated row was removed in the results | |
| assert feched_result == expected_result | |
| fetched_result = result.fetch(as_dict=True, order_by=('contrast', 'brightness')) | |
| Stimulus.delete_quick() | |
| assert fetched_result == expected_result |
Added test based off of the linked issue. It seems enforcing DISTINCT when we are making sql queries with primary keys was not useful, I have removed it for now.
fix #914