Deluan Quintão 63a5954e4f
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* fix(smartplaylist): use annotation index for playcount/rating/loved filters

Annotation-field criteria wrapped the column in COALESCE(col, default) so
missing annotation rows behave as 0/false. COALESCE prevents SQLite from
using the column index, forcing a full media_file scan during smart playlist
materialization - multi-second loads on large libraries, independent of rule
complexity.

Store the raw column plus its default and drop COALESCE when the compared
value cannot match the default; fall back to 'col <op> ? OR col IS NULL' when
the default would match, so never-annotated tracks are still preserved.
Sorting keeps COALESCE to retain deterministic NULL ordering. Result set is
unchanged; the materialize query now seeks the annotation index.

Signed-off-by: Deluan <deluan@deluan.com>

* fix(smartplaylist): keep COALESCE for list-valued annotation comparisons

Hardening from final review: a list value (IN (...)) can't drive the index
and a default-inclusive list has per-element NULL semantics, so route slice
values through COALESCE(col, default) to stay exactly equivalent to the prior
form. Also make the bool-default branch explicit (loved only supports
equality operators) and share the COALESCE rendering via coalesceExpr.

Signed-off-by: Deluan <deluan@deluan.com>

* refactor(smartplaylist): make coalesced() a field method

Thermo-nuclear review follow-up: promote the free coalesceExpr(f) to a
smartPlaylistField.coalesced() method that returns the bare expression when
there is no default. This lets sortExpr call field.coalesced() unconditionally
and drop its 'if coalesceDefault != nil' branch, removing the 'only annotation
fields get coalesced' special case from the sort path. Behavior unchanged.

Signed-off-by: Deluan <deluan@deluan.com>

* fix(smartplaylist): keep COALESCE for LIKE, bool-ordering, and tag ranges

Code review (xhigh) found the index-friendly rewrite did not cover every
operator, breaking result-set equivalence on a few reachable raw-JSON paths:

- LIKE family (contains/startsWith/endsWith/notContains) on annotation fields
  used the bare column, so a NULL column never matched and missing-annotation
  rows were dropped.
- Ordering comparators (gt/lt/...) on bool fields (loved) were decided as
  equality, wrongly including never-annotated rows.
- InTheRange on a numeric tag split into two independent json_tree EXISTS,
  letting different tag values satisfy each bound.

Centralize the decision in annotationCond via bareNullInclusion: emit the
index-friendly bare form only for scalar values under an exactly-orderable
comparator, otherwise fall back to the COALESCE form (always equivalent to the
original). Route LIKE through coalesced(); reject tag/role ranges. Replace the
local toFloat/toBool with spf13/cast (fixes unhandled numeric types and string
bool forms), and drop the redundant LookupField + double reflect.TypeOf.

A 17-case brute-force check confirms row-set equivalence to the prior
COALESCE form across all operators including the fixed LIKE/bool cases.

Signed-off-by: Deluan <deluan@deluan.com>

* fix(smartplaylist): keep COALESCE for list values on bool annotation fields

Second review found the bareNullInclusion bool branch missed the non-scalar
guard the numeric branch has: a list value on loved/albumloved/artistloved
(e.g. {"is":{"loved":[true]}}) coerced through toBool (which swallowed the
cast error) to false, emitting the bare/OR-IS-NULL form and wrongly including
never-annotated rows. Make toBool return (value, ok) like toFloat and bail to
COALESCE when the value isn't a scalar bool. Also add the missing test for the
tag/role range rejection. A 21-case brute-force confirms row-set equivalence to
the original COALESCE form across every operator, including the bool/numeric
list paths.

Signed-off-by: Deluan <deluan@deluan.com>

* refactor(smartplaylist): drop spf13/cast for stdlib value coercion

The value coercion only sees the handful of types criteria produces (int,
float64, string from JSON; bool already normalized at unmarshal), so cast's
broad conversion isn't needed. Use small explicit type switches over strconv
instead, keeping the string fallback (ParseFloat/ParseBool) that closes the
'1'/'t' gap. No dependency change — cast returns to indirect.

Signed-off-by: Deluan <deluan@deluan.com>

* refactor(smartplaylist): share bool coercion via criteria.ToBool

normalizeBoolValue (unmarshal-time) and the persistence bool guard both parsed
bool-ish values independently. Extract the shared logic into an exported
criteria.ToBool(any) (bool, ok): normalizeBoolValue delegates to it (behavior
unchanged), and the persistence layer reuses it via its existing model/criteria
import instead of a local helper. No behavior change.

Signed-off-by: Deluan <deluan@deluan.com>

* refactor(smartplaylist): trim sqlLiteral and dedup rationale comments

/simplify cleanup: fmt %v already renders bool defaults as false/true, so drop
sqlLiteral's redundant bool branch. Consolidate the COALESCE-vs-index rationale
to the smartPlaylistField comment instead of repeating it across annotationCond
and the struct. No behavior change.

Signed-off-by: Deluan <deluan@deluan.com>

* fix(smartplaylist): address review feedback on multi-field maps and *any

From the PR bot reviews:
- sqlFields now uses the field's coalesced() form, so annotation fields in a
  multi-field operator map (Is/Gt/Contains with >1 key) keep COALESCE and don't
  silently drop never-annotated rows. Covers both the comparison and LIKE
  fallback paths. (Gemini high, Copilot)
- Replace coalesceDefault *any with a plain any (0/false are non-nil
  interfaces, so nil still means 'no default'); drop the coalesce() boxing
  helper and the pointer indirection. (Gemini)
- Give rangeExpr clear, range-specific errors for the multi-field and malformed
  -pair cases instead of an empty-field / 'in operator' message. (Copilot)

Adds tests for the multi-field COALESCE behavior and the new range errors.

Signed-off-by: Deluan <deluan@deluan.com>

* Revert multi-field COALESCE handling (YAGNI)

The multi-field operator map case the bots flagged is unreachable: marshalExpression
rejects any operator map with more than one field, so a multi-field map can never be
persisted or loaded. Revert the sqlFields change and its tests rather than harden a
code path no supported input can reach. Keep the two reachable improvements from the
review: coalesceDefault any (not *any), and the clearer malformed-range error.

Signed-off-by: Deluan <deluan@deluan.com>

* refactor(persistence): model comparator as a behavior-carrying struct

The smart-playlist comparator was a bare string alias, forcing two parallel
switches over the same six operators: squirrelCmp mapped each to its squirrel
constructor, and bareNullInclusion restated each as a float predicate. Adding
or changing an operator meant editing both in sync.

Make comparator a struct that bundles those facts per operator (the squirrel
builder, the operator as a float predicate, and whether it's an ordering op).
Both switches collapse: squirrelCmp is deleted in favor of cmp.build, and
bareNullInclusion's numeric switch becomes a single cmp.satisfy call. Generated
SQL is unchanged, as the existing table-driven tests confirm.

* docs(smartplaylist): trim comments that restate the code

Remove or tighten comments that describe what the code already says (likeCond and
comparisonExpr doc lines, redundant clauses in annotationField/coalesced/ToBool/
normalizeBoolValue). Keep the comments that explain non-obvious rationale: the
COALESCE-vs-index tradeoff, the bareNullInclusion/annotationCond contracts, and the
why-we-fall-back notes.

* docs(smartplaylist): collapse coalesceDefault comment to one line

The field's six-line block duplicated the COALESCE-vs-index rationale that already
lives on annotationCond. Reduce it to a one-line description plus a pointer there.

---------

Signed-off-by: Deluan <deluan@deluan.com>
2026-06-24 22:26:41 -04:00

174 lines
3.6 KiB
Go

package criteria
import (
"encoding/json"
"fmt"
"strconv"
"strings"
)
type unmarshalConjunctionType []Expression
func (uc *unmarshalConjunctionType) UnmarshalJSON(data []byte) error {
var raw []map[string]json.RawMessage
if err := json.Unmarshal(data, &raw); err != nil {
return err
}
var es unmarshalConjunctionType
for _, e := range raw {
for k, v := range e {
k = strings.ToLower(k)
expr := unmarshalExpression(k, v)
if expr == nil {
expr = unmarshalConjunction(k, v)
}
if expr == nil {
return fmt.Errorf(`invalid expression key '%s'`, k)
}
es = append(es, expr)
}
}
*uc = es
return nil
}
func unmarshalExpression(opName string, rawValue json.RawMessage) Expression {
m := make(map[string]any)
err := json.Unmarshal(rawValue, &m)
if err != nil {
return nil
}
normalizeBoolFields(m)
switch opName {
case "is":
return Is(m)
case "isnot":
return IsNot(m)
case "gt":
return Gt(m)
case "lt":
return Lt(m)
case "contains":
return Contains(m)
case "notcontains":
return NotContains(m)
case "startswith":
return StartsWith(m)
case "endswith":
return EndsWith(m)
case "intherange":
return InTheRange(m)
case "before":
return Before(m)
case "after":
return After(m)
case "inthelast":
return InTheLast(m)
case "notinthelast":
return NotInTheLast(m)
case "inplaylist":
return InPlaylist(m)
case "notinplaylist":
return NotInPlaylist(m)
case "ismissing":
normalizeAllBoolFields(m)
return IsMissing(m)
case "ispresent":
normalizeAllBoolFields(m)
return IsPresent(m)
}
return nil
}
func normalizeAllBoolFields(m map[string]any) {
for k, v := range m {
m[k] = normalizeBoolValue(v)
}
}
func normalizeBoolFields(m map[string]any) {
for field, value := range m {
info, ok := LookupField(field)
if ok && info.Boolean {
m[field] = normalizeBoolValue(value)
}
}
}
// ToBool coerces a criteria value to a bool, accepting the forms criteria values take: a real bool,
// a strconv.ParseBool-parseable string, or a JSON number that is exactly 0 or 1. Any other value
// (other numbers, slices, nil, unparseable strings) returns ok=false so callers can handle it.
func ToBool(v any) (bool, bool) {
switch val := v.(type) {
case bool:
return val, true
case string:
b, err := strconv.ParseBool(val)
return b, err == nil
case float64:
switch val {
case 1:
return true, true
case 0:
return false, true
}
}
return false, false
}
// normalizeBoolValue leaves non-boolean values unchanged so they flow through to their own validation.
func normalizeBoolValue(v any) any {
if b, ok := ToBool(v); ok {
return b
}
return v
}
func unmarshalConjunction(conjName string, rawValue json.RawMessage) Expression {
var items unmarshalConjunctionType
err := json.Unmarshal(rawValue, &items)
if err != nil {
return nil
}
switch conjName {
case "any":
return Any(items)
case "all":
return All(items)
}
return nil
}
func marshalExpression(name string, value map[string]any) ([]byte, error) {
if len(value) != 1 {
return nil, fmt.Errorf(`invalid %s expression length %d for values %v`, name, len(value), value)
}
b := strings.Builder{}
b.WriteString(`{"` + name + `":{`)
for f, v := range value {
j, err := json.Marshal(v)
if err != nil {
return nil, err
}
b.WriteString(`"` + f + `":`)
b.Write(j)
break
}
b.WriteString("}}")
return []byte(b.String()), nil
}
func marshalConjunction(name string, conj []Expression) ([]byte, error) {
aux := struct {
All []Expression `json:"all,omitempty"`
Any []Expression `json:"any,omitempty"`
}{}
if name == "any" {
aux.Any = conj
} else {
aux.All = conj
}
return json.Marshal(aux)
}