diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index fa769ef40..b74f498d0 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -140,7 +140,7 @@ func (c smartPlaylistCriteria) exprSQL(expr criteria.Expression) (squirrel.Sqliz } and = append(and, cond) } - return and, nil + return mergeNegatedJsonConds(and), nil case criteria.Any: or := squirrel.Or{} for _, child := range e { @@ -454,6 +454,25 @@ const jsonCondBatchSize = 350 // This turns N separate correlated subqueries into ceil(N/batchSize), dramatically // improving performance for smart playlists with many patterns. func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { + if merged, ok := mergeSameFieldConds(or, false); ok { + return squirrel.Or(merged) + } + return or +} + +// mergeNegatedJsonConds is the AND-group counterpart to mergeJsonConds, merging negated +// conditions. By De Morgan, "NOT EXISTS(X) AND NOT EXISTS(Y)" == "NOT EXISTS(X OR Y)". +func mergeNegatedJsonConds(and squirrel.And) squirrel.Sqlizer { + if merged, ok := mergeSameFieldConds(and, true); ok { + return squirrel.And(merged) + } + return and +} + +// mergeSameFieldConds groups roleCond/tagCond entries that share a field and the requested +// polarity, replacing each group of 2+ with batched roleCondGroup/tagCondGroup subqueries. +// Returns the rewritten conditions and whether any merge happened. +func mergeSameFieldConds(conds []squirrel.Sqlizer, negated bool) ([]squirrel.Sqlizer, bool) { type condEntry struct { index int cond squirrel.Sqlizer @@ -465,10 +484,10 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { tag string } groups := make(map[string]*group) - for i, s := range or { + for i, s := range conds { switch c := s.(type) { case roleCond: - if c.not || c.cond == nil { + if c.not != negated || c.cond == nil { continue } g, exists := groups["role:"+c.role] @@ -478,7 +497,7 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { } g.entries = append(g.entries, condEntry{index: i, cond: c.cond}) case tagCond: - if c.not || c.cond == nil { + if c.not != negated || c.cond == nil { continue } g, exists := groups["tag:"+c.tag] @@ -490,7 +509,6 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { } } - merged := false remove := make(map[int]bool) var additions []squirrel.Sqlizer for _, key := range slices.Sorted(maps.Keys(groups)) { @@ -498,45 +516,42 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { if len(g.entries) < 2 { continue } - merged = true - for _, e := range g.entries { - remove[e.index] = true - } - conds := make([]squirrel.Sqlizer, len(g.entries)) + batchConds := make([]squirrel.Sqlizer, len(g.entries)) for i, e := range g.entries { - conds[i] = e.cond + remove[e.index] = true + batchConds[i] = e.cond } if g.isRole { role := key[len("role:"):] - for batch := range slices.Chunk(conds, jsonCondBatchSize) { - additions = append(additions, roleCondGroup{role: role, conds: batch}) + for batch := range slices.Chunk(batchConds, jsonCondBatchSize) { + additions = append(additions, roleCondGroup{role: role, conds: batch, not: negated}) } } else { - for batch := range slices.Chunk(conds, jsonCondBatchSize) { - additions = append(additions, tagCondGroup{tag: g.tag, numeric: g.numeric, conds: batch}) + for batch := range slices.Chunk(batchConds, jsonCondBatchSize) { + additions = append(additions, tagCondGroup{tag: g.tag, numeric: g.numeric, conds: batch, not: negated}) } } } - if !merged { - return or + if len(remove) == 0 { + return conds, false } - result := make(squirrel.Or, 0, len(or)-len(remove)+len(additions)) - for i, s := range or { + result := make([]squirrel.Sqlizer, 0, len(conds)-len(remove)+len(additions)) + for i, s := range conds { if !remove[i] { result = append(result, s) } } - result = append(result, additions...) - return result + return append(result, additions...), true } -// roleCondGroup represents multiple role conditions for the same role, merged into -// a single EXISTS subquery for performance. +// roleCondGroup represents multiple role conditions for the same role, merged into a single +// (optionally negated) EXISTS subquery for performance. type roleCondGroup struct { role string conds []squirrel.Sqlizer + not bool } func (g roleCondGroup) ToSql() (string, []any, error) { @@ -551,15 +566,19 @@ func (g roleCondGroup) ToSql() (string, []any, error) { allArgs = append(allArgs, args...) } cond := roleExistsSQL("(" + strings.Join(innerParts, " OR ") + ")") + if g.not { + cond = "not " + cond + } return cond, allArgs, nil } -// tagCondGroup represents multiple tag conditions for the same tag, merged into -// a single EXISTS subquery for performance. +// tagCondGroup represents multiple tag conditions for the same tag, merged into a single +// (optionally negated) EXISTS subquery for performance. type tagCondGroup struct { tag string numeric bool conds []squirrel.Sqlizer + not bool } func (g tagCondGroup) ToSql() (string, []any, error) { @@ -578,6 +597,9 @@ func (g tagCondGroup) ToSql() (string, []any, error) { } cond := fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value' and (%s))", g.tag, strings.Join(innerParts, " OR ")) + if g.not { + cond = "not " + cond + } return cond, allArgs, nil } diff --git a/persistence/criteria_sql_benchmark_test.go b/persistence/criteria_sql_benchmark_test.go index d901e9eda..1dcf97871 100644 --- a/persistence/criteria_sql_benchmark_test.go +++ b/persistence/criteria_sql_benchmark_test.go @@ -60,6 +60,61 @@ func BenchmarkSmartPlaylistRole(b *testing.B) { }) } +// BenchmarkSmartPlaylistNegatedRole compares performance for smart playlists with many +// negated role conditions ANDed together (e.g. 500 "isNot artist" rules, issue #5511) +// between the current implementation (merged NOT EXISTS via criteria pipeline) and the +// old baseline (one separate NOT EXISTS subquery per pattern). +func BenchmarkSmartPlaylistNegatedRole(b *testing.B) { + configtest.SetupConfig() + tmpDir := b.TempDir() + conf.Server.DbPath = filepath.Join(tmpDir, "bench-smartpl-neg.db") + cleanup := db.Init(context.Background()) + defer cleanup() + log.SetLevel(log.LevelFatal) + + conn := dbx.NewFromDB(db.Db(), db.Dialect) + ctx := log.NewContext(context.Background()) + user := model.User{ID: "bench-user", UserName: "bench", Name: "Bench User", IsAdmin: true} + ctx = request.WithUser(ctx, user) + + setupBenchData(b, ctx, conn, user) + criteria.AddRoles([]string{"artist"}) + + // Build the criteria expression: 500 "isNot artist" patterns in an AND group + allExprs := make(criteria.All, benchNumPatterns) + for i := range benchNumPatterns { + allExprs[i] = criteria.IsNot{"artist": fmt.Sprintf("Artist %04d", i)} + } + expr := criteria.Criteria{Expression: allExprs, Sort: "title", Limit: 500} + + b.Run("Current", func(b *testing.B) { + benchmarkCriteriaPipeline(b, ctx, expr) + }) + b.Run("Baseline_UnmergedNotExists", func(b *testing.B) { + benchmarkUnmergedNegatedJSONTree(b, ctx) + }) +} + +// benchmarkUnmergedNegatedJSONTree builds the old-style query with N separate negated +// json_tree EXISTS subqueries ANDed together (the pre-optimization baseline). +func benchmarkUnmergedNegatedJSONTree(b *testing.B, ctx context.Context) { + b.Helper() + + var sb strings.Builder + sb.WriteString("SELECT media_file.id FROM media_file WHERE (") + args := make([]any, 0, benchNumPatterns) + for i := range benchNumPatterns { + if i > 0 { + sb.WriteString(" AND ") + } + sb.WriteString("not exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name' and value = ?)") + args = append(args, fmt.Sprintf("Artist %04d", i)) + } + sb.WriteString(") ORDER BY media_file.title LIMIT 500") + + runBenchQuery(b, ctx, sb.String(), args) +} + // benchmarkCriteriaPipeline runs the criteria through the actual production code path: // newSmartPlaylistCriteria → Where() → ToSql(), then executes the resulting query. func benchmarkCriteriaPipeline(b *testing.B, ctx context.Context, expr criteria.Criteria) { diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 0257fa0cf..9ff7f1f07 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -408,6 +408,97 @@ var _ = Describe("Smart playlist criteria SQL", func() { Expect(sql).To(ContainSubstring("value LIKE ? OR value LIKE ?")) Expect(args).To(HaveLen(2 + 2 + 1)) // 2 tag patterns + 2 role patterns + 1 role name }) + + It("merges negated role conditions in an AND group into a single NOT EXISTS", func() { + expr := criteria.All{ + criteria.IsNot{"artist": "Beatles"}, + criteria.IsNot{"artist": "Kraftwerk"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + // A single NOT EXISTS with both names ORed inside (De Morgan) + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(sql).To(ContainSubstring("artist.name = ? OR artist.name = ?")) + Expect(args).To(HaveExactElements("artist", "Beatles", "Kraftwerk")) + }) + + It("merges negated notContains role conditions in an AND group", func() { + expr := criteria.All{ + criteria.NotContains{"artist": "Beatles"}, + criteria.NotContains{"artist": "Kraftwerk"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(sql).To(ContainSubstring("artist.name LIKE ? OR artist.name LIKE ?")) + Expect(args).To(HaveExactElements("artist", "%Beatles%", "%Kraftwerk%")) + }) + + It("merges negated tag conditions in an AND group into a single NOT EXISTS", func() { + expr := criteria.All{ + criteria.NotContains{"genre": "Rock"}, + criteria.NotContains{"genre": "Metal"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(sql).To(ContainSubstring("value LIKE ? OR value LIKE ?")) + Expect(args).To(HaveExactElements("%Rock%", "%Metal%")) + }) + + It("does not merge a single negated condition with a positive one of the same role in AND", func() { + // AND of mixed polarity must not be collapsed: NOT EXISTS(a) AND EXISTS(b) + // is not equivalent to any single merged subquery. + expr := criteria.All{ + criteria.Contains{"artist": "Beatles"}, + criteria.IsNot{"artist": "Kraftwerk"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, _, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + // One positive EXISTS and one negated NOT EXISTS, kept separate + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(strings.Count(sql, "exists")).To(Equal(2)) // "not exists" contains "exists" + }) + + It("does not merge negated conditions of different roles in AND", func() { + expr := criteria.All{ + criteria.IsNot{"artist": "Beatles"}, + criteria.IsNot{"composer": "Lennon"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, _, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Count(sql, "not exists")).To(Equal(2)) + }) + + It("batches large negated AND groups to avoid SQLite expression tree depth limit", func() { + allExprs := make(criteria.All, jsonCondBatchSize+1) + for i := range allExprs { + allExprs[i] = criteria.IsNot{"artist": fmt.Sprintf("Artist%d", i)} + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: allExprs}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + // Two NOT EXISTS subqueries (one batch of jsonCondBatchSize, one of 1) + Expect(strings.Count(sql, "not exists")).To(Equal(2)) + Expect(args).To(HaveLen(2 + jsonCondBatchSize + 1)) + }) }) Describe("joins", func() {