Sorting support in beets query mecanism (dbcore) #823

PierreRust · 2014-06-15T20:39:15Z

Add sorting support (see #749)

in the dbcore API and as a new syntax for queries
default sort order can be defined in the configuration file
documentation and tests case are update accordingly

Plugin-defined Sort objects are not yet implemented but this could be done later (and everything is in place for that).

* sort can be sepcified using the 'field_name'(+|-) syntax * supports fixed fields and flexible attributes * includes plugins fix for API changes (might have missed some)

Avoid newlines in the query string.

For items and albums, defaulting on artist field if artist field is empty.

Default sort order can be set in configuration file with the `sort_album` and `sort_item` keys.

Sort is implemented in python. When combining sorts on fixed, flex and computed field, make as much as possible in sql and finishes the job in python.

Fix crash when using several criteria for default sort in the configuration file.

SQL syntax error when using a simple FlexFieldSort.

On fixed and flex attributes.

sampsyo · 2014-06-17T14:18:56Z

Awesome! Sorry I'm being slow to review this; I'm traveling at the moment. Hope to get to this soon.

PierreRust · 2014-06-17T15:09:51Z

No worries, I'll try to find some time too to get back on the web API.

sampsyo · 2014-06-24T00:10:58Z

beets/dbcore/db.py

-            values = dict(row)
-            flex_values = dict((row['key'], row['value']) for row in flex_rows)
+        if self.sort:
+            # slow sort, must build the full list first


This is a really minor consistency issue, but the current beets style uses "full sentences" in comments. That is:

# Slow sort. Must build the full list first.

with capital letters and periods.

sampsyo · 2014-06-24T00:21:09Z

This is looking great! Thanks again for attacking such a huge task.

I made a few very low-level comments on style, mainly about source formatting. It also looks like flake8 is suggesting two tiny whitespace changes: https://travis-ci.org/sampsyo/beets/jobs/27636028

I really like the simplicity of extending get_query to return a Sort object also. I hope we've found all the places that need to be adapted for this change.

My only design-level comment is this: could we reduce the complexity of sorting if we always eagerly fetched all the flexible attributes with a JOIN? That is, if we were to modify queries to always join on the flexattr tables (even when not sorting), could we avoid the conditional inclusion select_clause and union_clause and thereby simplify the sorting code? (Let me know if this question came out too garbled to understand. 😃)

PierreRust · 2014-07-02T11:19:41Z

I've just committed the small commenting issues.

I'd love to reduce sorting complexity and fetch all flexattr (that could probably be great for performance too). But I do not see how I could write an sql query to do that if I don't know the key for the flexattr. With sorting it's easier as we know the key of the flexattr we need to fetch.

PierreRust · 2014-07-08T10:40:04Z

Formatting should be fine now. The build fails but it seems to be a problem on travis side.

geigerzaehler · 2014-07-08T15:18:43Z

beets/dbcore/db.py

+                if not self.query or self.query.match(obj):
+                    yield obj
+
+    def _generate_results(self, row):


_make_model might be a more descriptive name.

no problem !

geigerzaehler · 2014-07-08T15:37:15Z

beets/library.py

+        query string the sort_order argument is ignored.
+          """
+        (query, sort) = get_query(query, model_cls)
+        sort = sort_order if sort is None else sort


I think this would be more readable:

if not sort: sort = sort_order

the variable = x if ... else z construct does not seem to get much love from python developers ;)

Personnaly I don't really care, I can change that.

the variable = x if ... else z construct does not seem to get much love from python developers ;)

Indeed.

To elaborate: In the case that sort is not none, the original statement is actually a no-op. And that is the whole point of the statement: assigning a fallback value. The alternative conveys that more clearly.

FWIW, the idiom I see (and use) most often is:

sort = sort or sort_order

which, while "clever", is readable to me because it is idiomatic.

One more style nitpick: parentheses are not required on the previous line.

geigerzaehler · 2014-07-08T15:42:55Z

Great work @PierreRust! I've added some comments and suggestions.

geigerzaehler · 2014-07-10T11:30:14Z

Formatting should be fine now. The build fails but it seems to be a problem on travis side.

If this happens you can rerun the build on travis. It now passes.

sampsyo · 2014-07-10T18:57:49Z

Yes, looking awesome. If we're down to the syntax details, maybe it's nearly time to merge! Want to hit the big green button when you feel it's ready?

PierreRust · 2014-07-11T07:26:24Z

I'll try to make the small changes today at lunch time, but after that, I'm going on vacation for one week (yeah !) with no pc.

PierreRust · 2014-07-11T11:38:09Z

Ok, I won't have time to make all changes before vacation and test properly, I'm afraid it will have to wait a bit more :(

sampsyo · 2014-07-11T16:07:41Z

Have a great vacation! I may get around to pulling this into a branch in the mean time.

PierreRust · 2014-07-24T12:03:25Z

I've made all the changes except changing Sort to make it callable.

geigerzaehler · 2014-07-28T10:18:55Z

Looks good to me.

PierreRust · 2014-07-28T15:47:28Z

@geigerzaehler : I'm just wondering what you had in mind in your comment about passing a function as sort argument ?

Otherwise, I'd be happy to click on this big green button and merge it in the main branch :)

geigerzaehler · 2014-07-28T18:06:01Z

@geigerzaehler : I'm just wondering what you had in mind in your comment about passing a function as sort argument ?

Just an idea that sprang to my mind. But I see no real use right now.

sampsyo · 2014-07-28T19:09:04Z

Okay, cool! Since this is a relatively large change, how do you both feel about pushing out a new stable release posthaste and then merging this as the main change for the next version? That will give us some freedom to refine this sorting stuff over the next release cycle (and give it the attention it deserves).

PierreRust · 2014-07-28T21:27:32Z

Sound like a good idea imo, I've tried to be careful but there are certainly some bugs hidden there. It's safer to release a stable version before merging that.

sampsyo · 2014-08-22T21:59:42Z

Sorry for the long delay, but I just pushed out 1.3.7 so I think I'll merge this now. I'll keep fiddling with it, so questions may still come up in the future. But THANK YOU for tackling this enormous issue! It will make for a great next release (and may it come sooner rather than later).

Sorting support in beets query mecanism (dbcore)

PierreRust added 15 commits June 15, 2014 22:32

Sort implementation

1303a91

* sort can be sepcified using the 'field_name'(+|-) syntax * supports fixed fields and flexible attributes * includes plugins fix for API changes (might have missed some)

Use implicit continuation for sql generation.

3d7814c

Avoid newlines in the query string.

Add defaut sort on artist_sort field.

4958ce8

For items and albums, defaulting on artist field if artist field is empty.

Add documentation about sort specification.

3426363

Moves default sort specification from code to conf

86c34f8

Default sort order can be set in configuration file with the `sort_album` and `sort_item` keys.

Add documentation about default sort configuration.

486289b

Sort on computed field implementation.

1d61088

Sort is implemented in python. When combining sorts on fixed, flex and computed field, make as much as possible in sql and finishes the job in python.

Fix error on default sort configuration

ebd2da1

Fix crash when using several criteria for default sort in the configuration file.

Removes unused import (flake8 warning)

a90991b

Fix sorting without using the MultipleSort object.

1a995ed

SQL syntax error when using a simple FlexFieldSort.

Add unit tests for fixef and flex field sort

ad1c0b3

Add tests cases for sort on albums.

7287977

On fixed and flex attributes.

Fix typo in common utility function for tests

936acf0

Add tests for computed attributes.

69213ad

Fix flake8 warnings.

18539b0

sampsyo reviewed Jun 24, 2014
View reviewed changes

Fix minor formatting and comments issues.

cd1dcf6

PierreRust added 2 commits July 2, 2014 19:06

Fix formatting issue

451f17c

Fix formatting (again ...)

d168c62

geigerzaehler reviewed Jul 8, 2014
View reviewed changes

Minor changes from review (mostly style)

3130a6a

sampsyo added a commit that referenced this pull request Aug 22, 2014

Merge pull request #823 from PierreRust/sortquery

eb579cf

Sorting support in beets query mecanism (dbcore)

sampsyo merged commit eb579cf into beetbox:master Aug 22, 2014

sampsyo added a commit that referenced this pull request Aug 22, 2014

Changelog for sorting (#823)

132fad8

sampsyo added this to the 1.3.8: Sorting milestone Aug 25, 2014

sampsyo mentioned this pull request Sep 13, 2014

Finish sorting #953

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sorting support in beets query mecanism (dbcore) #823

Sorting support in beets query mecanism (dbcore) #823

PierreRust commented Jun 15, 2014

sampsyo commented Jun 17, 2014

PierreRust commented Jun 17, 2014

sampsyo Jun 24, 2014

sampsyo commented Jun 24, 2014

PierreRust commented Jul 2, 2014

PierreRust commented Jul 8, 2014

geigerzaehler Jul 8, 2014

PierreRust Jul 8, 2014

geigerzaehler Jul 8, 2014

PierreRust Jul 8, 2014

geigerzaehler Jul 10, 2014

sampsyo Jul 10, 2014

geigerzaehler commented Jul 8, 2014

geigerzaehler commented Jul 10, 2014

sampsyo commented Jul 10, 2014

PierreRust commented Jul 11, 2014

PierreRust commented Jul 11, 2014

sampsyo commented Jul 11, 2014

PierreRust commented Jul 24, 2014

geigerzaehler commented Jul 28, 2014

PierreRust commented Jul 28, 2014

geigerzaehler commented Jul 28, 2014

sampsyo commented Jul 28, 2014

PierreRust commented Jul 28, 2014

sampsyo commented Aug 22, 2014

Sorting support in beets query mecanism (dbcore) #823

Sorting support in beets query mecanism (dbcore) #823

Conversation

PierreRust commented Jun 15, 2014

sampsyo commented Jun 17, 2014

PierreRust commented Jun 17, 2014

sampsyo Jun 24, 2014

Choose a reason for hiding this comment

sampsyo commented Jun 24, 2014

PierreRust commented Jul 2, 2014

PierreRust commented Jul 8, 2014

geigerzaehler Jul 8, 2014

Choose a reason for hiding this comment

PierreRust Jul 8, 2014

Choose a reason for hiding this comment

geigerzaehler Jul 8, 2014

Choose a reason for hiding this comment

PierreRust Jul 8, 2014

Choose a reason for hiding this comment

geigerzaehler Jul 10, 2014

Choose a reason for hiding this comment

sampsyo Jul 10, 2014

Choose a reason for hiding this comment

geigerzaehler commented Jul 8, 2014

geigerzaehler commented Jul 10, 2014

sampsyo commented Jul 10, 2014

PierreRust commented Jul 11, 2014

PierreRust commented Jul 11, 2014

sampsyo commented Jul 11, 2014

PierreRust commented Jul 24, 2014

geigerzaehler commented Jul 28, 2014

PierreRust commented Jul 28, 2014

geigerzaehler commented Jul 28, 2014

sampsyo commented Jul 28, 2014

PierreRust commented Jul 28, 2014

sampsyo commented Aug 22, 2014