From 68eee96c030ebbd718d4d8ad2869afc2e54ddbf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Thu, 25 Apr 2024 17:26:07 +0100 Subject: [PATCH 1/4] Define InQuery and use it in PlaylistQuery While working on the DB optimisation I discovered one query which does not follow the 'FieldQuery' interface - 'PlaylistQuery', so I looked into it in more detail. One special thing about it is that it uses 'IN' SQL operator, so I defined 'InQuery' query class to have this logic outside of the playlist context. Otherwise, it seems like 'PlaylistQuery' is a field query, even if it has a very special way of resolving values it wants to query. In the future, we may want to consider moving this kind of custom _initialisation_ logic away from '__init__' methods to factory/@classmethod: this should make it more clear that the purpose of such logic is to resolve the data that is required to define a particular FieldQuery class fully. --- beets/dbcore/query.py | 29 ++++++++++++++++++++++++----- beetsplug/playlist.py | 26 +++++++++++--------------- 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py index 890acbe72..e2e5399cf 100644 --- a/beets/dbcore/query.py +++ b/beets/dbcore/query.py @@ -12,8 +12,7 @@ # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. -"""The Query type hierarchy for DBCore. -""" +"""The Query type hierarchy for DBCore.""" from __future__ import annotations @@ -155,9 +154,7 @@ class FieldQuery(Query, Generic[P]): @classmethod def value_match(cls, pattern: P, value: Any): - """Determine whether the value matches the pattern. Both - arguments are strings. - """ + """Determine whether the value matches the pattern.""" raise NotImplementedError() def match(self, obj: Model) -> bool: @@ -428,6 +425,28 @@ class NumericQuery(FieldQuery): return "1", () +class InQuery(FieldQuery[Sequence[AnySQLiteType]]): + """Query which matches values in the given set.""" + + field: str + pattern: Sequence[AnySQLiteType] + fast: bool = True + + @property + def subvals(self) -> Sequence[AnySQLiteType]: + return self.pattern + + def col_clause(self) -> Tuple[str, Sequence[AnySQLiteType]]: + placeholders = ", ".join(["?"] * len(self.subvals)) + return f"{self.field} IN ({placeholders})", self.subvals + + @classmethod + def value_match( + cls, pattern: Sequence[AnySQLiteType], value: AnySQLiteType + ) -> bool: + return value in pattern + + class CollectionQuery(Query): """An abstract query class that aggregates other queries. Can be indexed like a list to access the sub-queries. diff --git a/beetsplug/playlist.py b/beetsplug/playlist.py index d40f4125f..401178553 100644 --- a/beetsplug/playlist.py +++ b/beetsplug/playlist.py @@ -15,17 +15,18 @@ import fnmatch import os import tempfile -from typing import Any, Optional, Sequence, Tuple +from typing import Sequence import beets +from beets.dbcore.query import InQuery +from beets.library import BLOB_TYPE from beets.util import path_as_posix -class PlaylistQuery(beets.dbcore.NamedQuery): +class PlaylistQuery(InQuery): """Matches files listed by a playlist file.""" - def __init__(self, pattern): - self.pattern = pattern + def __init__(self, _, pattern: str, __): config = beets.config["playlist"] # Get the full path to the playlist @@ -39,7 +40,7 @@ class PlaylistQuery(beets.dbcore.NamedQuery): ), ) - self.paths = [] + paths = [] for playlist_path in playlist_paths: if not fnmatch.fnmatch(playlist_path, "*.[mM]3[uU]"): # This is not am M3U playlist, skip this candidate @@ -63,23 +64,18 @@ class PlaylistQuery(beets.dbcore.NamedQuery): # ignore comments, and extm3u extension continue - self.paths.append( + paths.append( beets.util.normpath( os.path.join(relative_to, line.rstrip()) ) ) f.close() break + super().__init__("path", paths) - def clause(self) -> Tuple[Optional[str], Sequence[Any]]: - if not self.paths: - # Playlist is empty - return "0", () - clause = "path IN ({})".format(", ".join("?" for path in self.paths)) - return clause, (beets.library.BLOB_TYPE(p) for p in self.paths) - - def match(self, item): - return item.path in self.paths + @property + def subvals(self) -> Sequence[BLOB_TYPE]: + return [BLOB_TYPE(p) for p in self.pattern] class PlaylistPlugin(beets.plugins.BeetsPlugin): From a57c164348b7ff49e2d5cd4d754dae5a1dee39e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Sun, 28 Apr 2024 17:43:00 +0100 Subject: [PATCH 2/4] Remove NamedQuery Remove 'NamedQuery' since it is not being used by any queries any more. This simplifies query parsing logic in 'queryparse.py'. We know that this logic can only receive 'FieldQuery' thus I adjusted types and removed the logic that handles other cases. Effectively, this means that the query parsing logic does not any more care whether the query is named by the corresponding DB field. Instead, queries like 'SingletonQuery' and 'PlaylistQuery' are responsible for translating 'singleton' and 'playlist' to the underlying DB filters. --- beets/dbcore/__init__.py | 1 - beets/dbcore/db.py | 8 +++----- beets/dbcore/query.py | 9 --------- beets/dbcore/queryparse.py | 38 ++++++++++++-------------------------- test/test_dbcore.py | 9 ++------- 5 files changed, 17 insertions(+), 48 deletions(-) diff --git a/beets/dbcore/__init__.py b/beets/dbcore/__init__.py index baeb10d26..06d0b3dc9 100644 --- a/beets/dbcore/__init__.py +++ b/beets/dbcore/__init__.py @@ -22,7 +22,6 @@ from .query import ( FieldQuery, InvalidQueryError, MatchQuery, - NamedQuery, OrQuery, Query, ) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index 7fbf646dc..369b1ffe0 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -12,8 +12,7 @@ # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. -"""The central Model and Database constructs for DBCore. -""" +"""The central Model and Database constructs for DBCore.""" from __future__ import annotations @@ -309,7 +308,7 @@ class Model(ABC): are subclasses of `Sort`. """ - _queries: Dict[str, Type[Query]] = {} + _queries: Dict[str, Type[FieldQuery]] = {} """Named queries that use a field-like `name:value` syntax but which do not relate to any specific field. """ @@ -599,8 +598,7 @@ class Model(ABC): # Deleted flexible attributes. for key in self._dirty: tx.mutate( - "DELETE FROM {} " - "WHERE entity_id=? AND key=?".format(self._flex_table), + f"DELETE FROM {self._flex_table} WHERE entity_id=? AND key=?", (self.id, key), ) diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py index e2e5399cf..b97d4999b 100644 --- a/beets/dbcore/query.py +++ b/beets/dbcore/query.py @@ -115,15 +115,6 @@ class Query(ABC): return hash(type(self)) -class NamedQuery(Query): - """Non-field query, i.e. the query prefix is not a field but identifies the - query class. - """ - - @abstractmethod - def __init__(self, pattern): ... - - P = TypeVar("P") SQLiteType = Union[str, float, int, memoryview] AnySQLiteType = TypeVar("AnySQLiteType", bound=SQLiteType) diff --git a/beets/dbcore/queryparse.py b/beets/dbcore/queryparse.py index e2b082ecc..ea6f16922 100644 --- a/beets/dbcore/queryparse.py +++ b/beets/dbcore/queryparse.py @@ -12,15 +12,14 @@ # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. -"""Parsing of strings into DBCore queries. -""" +"""Parsing of strings into DBCore queries.""" import itertools import re from typing import Collection, Dict, List, Optional, Sequence, Tuple, Type from . import Model, query -from .query import Query, Sort +from .query import Sort PARSE_QUERY_PART_REGEX = re.compile( # Non-capturing optional segment for the keyword. @@ -36,10 +35,10 @@ PARSE_QUERY_PART_REGEX = re.compile( def parse_query_part( part: str, - query_classes: Dict = {}, + query_classes: Dict[str, Type[query.FieldQuery]] = {}, prefixes: Dict = {}, default_class: Type[query.SubstringQuery] = query.SubstringQuery, -) -> Tuple[Optional[str], str, Type[query.Query], bool]: +) -> Tuple[Optional[str], str, Type[query.FieldQuery], bool]: """Parse a single *query part*, which is a chunk of a complete query string representing a single criterion. @@ -128,7 +127,7 @@ def construct_query_part( # Use `model_cls` to build up a map from field (or query) names to # `Query` classes. - query_classes: Dict[str, Type[Query]] = {} + query_classes: Dict[str, Type[query.FieldQuery]] = {} for k, t in itertools.chain( model_cls._fields.items(), model_cls._types.items() ): @@ -143,30 +142,17 @@ def construct_query_part( # If there's no key (field name) specified, this is a "match # anything" query. if key is None: - if issubclass(query_class, query.FieldQuery): - # The query type matches a specific field, but none was - # specified. So we use a version of the query that matches - # any field. - out_query = query.AnyFieldQuery( - pattern, model_cls._search_fields, query_class - ) - elif issubclass(query_class, query.NamedQuery): - # Non-field query type. - out_query = query_class(pattern) - else: - assert False, "Unexpected query type" + # The query type matches a specific field, but none was + # specified. So we use a version of the query that matches + # any field. + out_query = query.AnyFieldQuery( + pattern, model_cls._search_fields, query_class + ) # Field queries get constructed according to the name of the field # they are querying. - elif issubclass(query_class, query.FieldQuery): - key = key.lower() - out_query = query_class(key.lower(), pattern, key in model_cls._fields) - - # Non-field (named) query. - elif issubclass(query_class, query.NamedQuery): - out_query = query_class(pattern) else: - assert False, "Unexpected query type" + out_query = query_class(key.lower(), pattern, key in model_cls._fields) # Apply negation. if negate: diff --git a/test/test_dbcore.py b/test/test_dbcore.py index e5ab1910b..763601b7f 100644 --- a/test/test_dbcore.py +++ b/test/test_dbcore.py @@ -12,8 +12,7 @@ # The above copyright notice and this permission notice shall be # included in all copies or substantial portions of the Software. -"""Tests for the DBCore database abstraction. -""" +"""Tests for the DBCore database abstraction.""" import os import shutil @@ -32,7 +31,7 @@ class SortFixture(dbcore.query.FieldSort): pass -class QueryFixture(dbcore.query.NamedQuery): +class QueryFixture(dbcore.query.FieldQuery): def __init__(self, pattern): self.pattern = pattern @@ -605,10 +604,6 @@ class QueryFromStringsTest(unittest.TestCase): q = self.qfs([""]) self.assertIsInstance(q.subqueries[0], dbcore.query.TrueQuery) - def test_parse_named_query(self): - q = self.qfs(["some_query:foo"]) - self.assertIsInstance(q.subqueries[0], QueryFixture) - class SortFromStringsTest(unittest.TestCase): def sfs(self, strings): From 7d636d8f22659e99a08e56fbf047db07415d9490 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Tue, 30 Apr 2024 12:59:01 +0100 Subject: [PATCH 3/4] Add support for a specific type in InQuery --- beets/dbcore/query.py | 8 ++++---- beets/util/__init__.py | 2 +- beetsplug/playlist.py | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py index b97d4999b..ffa89168e 100644 --- a/beets/dbcore/query.py +++ b/beets/dbcore/query.py @@ -116,7 +116,7 @@ class Query(ABC): P = TypeVar("P") -SQLiteType = Union[str, float, int, memoryview] +SQLiteType = Union[str, bytes, float, int, memoryview] AnySQLiteType = TypeVar("AnySQLiteType", bound=SQLiteType) @@ -416,7 +416,7 @@ class NumericQuery(FieldQuery): return "1", () -class InQuery(FieldQuery[Sequence[AnySQLiteType]]): +class InQuery(Generic[AnySQLiteType], FieldQuery[Sequence[AnySQLiteType]]): """Query which matches values in the given set.""" field: str @@ -424,10 +424,10 @@ class InQuery(FieldQuery[Sequence[AnySQLiteType]]): fast: bool = True @property - def subvals(self) -> Sequence[AnySQLiteType]: + def subvals(self) -> Sequence[SQLiteType]: return self.pattern - def col_clause(self) -> Tuple[str, Sequence[AnySQLiteType]]: + def col_clause(self) -> Tuple[str, Sequence[SQLiteType]]: placeholders = ", ".join(["?"] * len(self.subvals)) return f"{self.field} IN ({placeholders})", self.subvals diff --git a/beets/util/__init__.py b/beets/util/__init__.py index ccb95b459..4335e0f3e 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -427,7 +427,7 @@ def displayable_path( return path.decode("utf-8", "ignore") -def syspath(path: bytes, prefix: bool = True) -> Bytes_or_String: +def syspath(path: Bytes_or_String, prefix: bool = True) -> Bytes_or_String: """Convert a path for use by the operating system. In particular, paths on Windows must receive a magic prefix and must be converted to Unicode before they are sent to the OS. To disable the magic diff --git a/beetsplug/playlist.py b/beetsplug/playlist.py index 401178553..83f95796e 100644 --- a/beetsplug/playlist.py +++ b/beetsplug/playlist.py @@ -23,9 +23,13 @@ from beets.library import BLOB_TYPE from beets.util import path_as_posix -class PlaylistQuery(InQuery): +class PlaylistQuery(InQuery[bytes]): """Matches files listed by a playlist file.""" + @property + def subvals(self) -> Sequence[BLOB_TYPE]: + return [BLOB_TYPE(p) for p in self.pattern] + def __init__(self, _, pattern: str, __): config = beets.config["playlist"] @@ -73,10 +77,6 @@ class PlaylistQuery(InQuery): break super().__init__("path", paths) - @property - def subvals(self) -> Sequence[BLOB_TYPE]: - return [BLOB_TYPE(p) for p in self.pattern] - class PlaylistPlugin(beets.plugins.BeetsPlugin): item_queries = {"playlist": PlaylistQuery} From b5e98b59f22f1bcef77d1a8226f4abb862429881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Tue, 30 Apr 2024 12:59:59 +0100 Subject: [PATCH 4/4] Configure mypy to require types for generics --- .mypy.ini | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.mypy.ini b/.mypy.ini index b47e5dff3..6bad7a0b6 100644 --- a/.mypy.ini +++ b/.mypy.ini @@ -1,5 +1,5 @@ [mypy] +allow_any_generics = false # FIXME: Would be better to actually type the libraries (if under our control), # or write our own stubs. For now, silence errors -ignore_missing_imports = True - +ignore_missing_imports = true