diff --git a/integration/Taskfile.yml b/integration/Taskfile.yml index 0550a7c6af97..3c79f0e03e91 100644 --- a/integration/Taskfile.yml +++ b/integration/Taskfile.yml @@ -93,7 +93,7 @@ tasks: - ../bin/benchstat{{exeExt}} old-sqlite.txt new-sqlite.txt bench-sqlite-no-pushdown: - desc: "Run benchmarks for `sqlite` with query pushdown disabled" + desc: "Run benchmarks for `sqlite` with filter pushdown disabled" cmds: - > go test -tags={{.BUILD_TAGS}} -timeout=0 -run=XXX diff --git a/integration/aggregate_documents_compat_test.go b/integration/aggregate_documents_compat_test.go index 34a63d9eda10..ae2fef21783b 100644 --- a/integration/aggregate_documents_compat_test.go +++ b/integration/aggregate_documents_compat_test.go @@ -104,26 +104,6 @@ func testAggregateStagesCompatWithProviders(t *testing.T, providers shareddata.P t.Run(targetCollection.Name(), func(t *testing.T) { t.Helper() - explainCommand := bson.D{{"explain", bson.D{ - {"aggregate", targetCollection.Name()}, - {"pipeline", pipeline}, - }}} - var explainRes bson.D - require.NoError(t, targetCollection.Database().RunCommand(ctx, explainCommand).Decode(&explainRes)) - - resultPushdown := tc.resultPushdown - - var msg string - // TODO https://github.com/FerretDB/FerretDB/issues/3386 - if setup.FilterPushdownDisabled() { - resultPushdown = noPushdown - msg = "Query pushdown is disabled, but target resulted with pushdown" - } - - doc := ConvertDocument(t, explainRes) - pushdown, _ := doc.Get("pushdown") - assert.Equal(t, resultPushdown.FilterPushdownExpected(t), pushdown, msg) - targetCursor, targetErr := targetCollection.Aggregate(ctx, pipeline, opts) compatCursor, compatErr := compatCollection.Aggregate(ctx, pipeline, opts) @@ -153,6 +133,26 @@ func testAggregateStagesCompatWithProviders(t *testing.T, providers shareddata.P if len(targetRes) > 0 || len(compatRes) > 0 { nonEmptyResults = true } + + explainCommand := bson.D{{"explain", bson.D{ + {"aggregate", targetCollection.Name()}, + {"pipeline", pipeline}, + }}} + var explainRes bson.D + require.NoError(t, targetCollection.Database().RunCommand(ctx, explainCommand).Decode(&explainRes)) + + resPushdown := tc.resultPushdown + + var msg string + // TODO https://github.com/FerretDB/FerretDB/issues/3386 + if setup.FilterPushdownDisabled() { + resPushdown = noPushdown + msg = "Fitler pushdown is disabled, but target resulted with pushdown" + } + + doc := ConvertDocument(t, explainRes) + pushdown, _ := doc.Get("filterPushdown") + assert.Equal(t, resPushdown.FilterPushdownExpected(t), pushdown, msg) }) } diff --git a/integration/query_command_compat_test.go b/integration/query_command_compat_test.go index 0225623e6095..ba24403cd624 100644 --- a/integration/query_command_compat_test.go +++ b/integration/query_command_compat_test.go @@ -37,7 +37,7 @@ type queryCommandCompatTestCase struct { optSkip any // defaults to nil to leave unset limit *int64 // defaults to nil to leave unset resultType compatTestCaseResultType // defaults to nonEmptyResult - resultPushdown resultPushdown // defaults to noPushdown + filterPushdown resultPushdown // defaults to noPushdown skip string // skip test for all handlers, must have issue number mentioned } @@ -103,13 +103,13 @@ func testQueryCommandCompat(t *testing.T, testCases map[string]queryCommandCompa var msg string if setup.FilterPushdownDisabled() { - tc.resultPushdown = noPushdown - msg = "Query pushdown is disabled, but target resulted with pushdown" + tc.filterPushdown = noPushdown + msg = "Filter pushdown is disabled, but target resulted with pushdown" } doc := ConvertDocument(t, explainRes) - pushdown, _ := doc.Get("pushdown") - assert.Equal(t, tc.resultPushdown.FilterPushdownExpected(t), pushdown, msg) + pushdown, _ := doc.Get("filterPushdown") + assert.Equal(t, tc.filterPushdown.FilterPushdownExpected(t), pushdown, msg) targetCommand := append( bson.D{ diff --git a/integration/query_compat_test.go b/integration/query_compat_test.go index 33c1e2c7ac31..93975fb693b5 100644 --- a/integration/query_compat_test.go +++ b/integration/query_compat_test.go @@ -133,11 +133,11 @@ func testQueryCompatWithProviders(t *testing.T, providers shareddata.Providers, var msg string if setup.FilterPushdownDisabled() { resultPushdown = noPushdown - msg = "Query pushdown is disabled, but target resulted with pushdown" + msg = "Filter pushdown is disabled, but target resulted with pushdown" } doc := ConvertDocument(t, explainRes) - pushdown, _ := doc.Get("pushdown") + pushdown, _ := doc.Get("filterPushdown") assert.Equal(t, resultPushdown.FilterPushdownExpected(t), pushdown, msg) targetCursor, targetErr := targetCollection.Find(ctx, filter, opts) @@ -242,29 +242,29 @@ func TestQueryCappedCollectionCompat(t *testing.T) { filter bson.D sort bson.D - unsafeSortPushdown resultPushdown + sortPushdown resultPushdown }{ "NoSortNoFilter": { - unsafeSortPushdown: allPushdown, + sortPushdown: allPushdown, }, "Filter": { - filter: bson.D{{"v", int32(42)}}, - unsafeSortPushdown: allPushdown, + filter: bson.D{{"v", int32(42)}}, + sortPushdown: allPushdown, }, "Sort": { - sort: bson.D{{"_id", int32(-1)}}, - unsafeSortPushdown: pgPushdown, + sort: bson.D{{"_id", int32(-1)}}, + sortPushdown: pgPushdown, }, "FilterSort": { - filter: bson.D{{"v", int32(42)}}, - sort: bson.D{{"_id", int32(-1)}}, - unsafeSortPushdown: pgPushdown, + filter: bson.D{{"v", int32(42)}}, + sort: bson.D{{"_id", int32(-1)}}, + sortPushdown: pgPushdown, }, "MultipleSortFields": { sort: bson.D{{"v", 1}, {"_id", int32(-1)}}, // multiple sort fields are skipped by handler and no sort pushdown // is set on handler, so record ID pushdown is done. - unsafeSortPushdown: allPushdown, + sortPushdown: allPushdown, }, } { name, tc := name, tc @@ -288,8 +288,8 @@ func TestQueryCappedCollectionCompat(t *testing.T) { require.NoError(t, targetCollection.Database().RunCommand(ctx, bson.D{{"explain", explainQuery}}).Decode(&explainRes)) doc := ConvertDocument(t, explainRes) - unsafeSortPushdown, _ := doc.Get("sortingPushdown") - assert.Equal(t, tc.unsafeSortPushdown.SortPushdownExpected(t, true), unsafeSortPushdown) + sortPushdown, _ := doc.Get("sortPushdown") + assert.Equal(t, tc.sortPushdown.SortPushdownExpected(t, true), sortPushdown) findOpts := options.Find() if tc.sort != nil { diff --git a/integration/query_test.go b/integration/query_test.go index 948088612617..e763ed820c31 100644 --- a/integration/query_test.go +++ b/integration/query_test.go @@ -821,10 +821,10 @@ func TestQueryCommandSingleBatch(t *testing.T) { } } -func TestQueryCommandUnsafeLimitPushDown(t *testing.T) { +func TestQueryCommandLimitPushDown(t *testing.T) { t.Parallel() - // must use a collection of documents which does not support query pushdown to test limit pushdown + // must use a collection of documents which does not support filter pushdown to test limit pushdown s := setup.SetupWithOpts(t, &setup.SetupOpts{Providers: []shareddata.Provider{shareddata.Composites}}) ctx, collection := s.Ctx, s.Collection @@ -834,117 +834,117 @@ func TestQueryCommandUnsafeLimitPushDown(t *testing.T) { sort bson.D // optional, nil to leave sort unset optSkip *int64 // optional, nil to leave optSkip unset - len int // expected length of results - queryPushdown resultPushdown // optional, defaults to noPushdown - unsafeLimitPushdown bool // optional, set true for expected pushdown for limit - err *mongo.CommandError // optional, expected error from MongoDB - altMessage string // optional, alternative error message for FerretDB, ignored if empty - skip string // optional, skip test with a specified reason - failsForSQLite string // optional, if set, the case is expected to fail for SQLite due to given issue + len int // expected length of results + filterPushdown resultPushdown // optional, defaults to noPushdown + limitPushdown resultPushdown // optional, defaults to noPushdown + err *mongo.CommandError // optional, expected error from MongoDB + altMessage string // optional, alternative error message for FerretDB, ignored if empty + skip string // optional, skip test with a specified reason + failsForSQLite string // optional, if set, the case is expected to fail for SQLite due to given issue }{ "Simple": { - limit: 1, - len: 1, - unsafeLimitPushdown: true, + limit: 1, + len: 1, + limitPushdown: allPushdown, }, "AlmostAll": { - limit: int64(len(shareddata.Composites.Docs()) - 1), - len: len(shareddata.Composites.Docs()) - 1, - unsafeLimitPushdown: true, + limit: int64(len(shareddata.Composites.Docs()) - 1), + len: len(shareddata.Composites.Docs()) - 1, + limitPushdown: allPushdown, }, "All": { - limit: int64(len(shareddata.Composites.Docs())), - len: len(shareddata.Composites.Docs()), - unsafeLimitPushdown: true, + limit: int64(len(shareddata.Composites.Docs())), + len: len(shareddata.Composites.Docs()), + limitPushdown: allPushdown, }, "More": { - limit: int64(len(shareddata.Composites.Docs()) + 1), - len: len(shareddata.Composites.Docs()), - unsafeLimitPushdown: true, + limit: int64(len(shareddata.Composites.Docs()) + 1), + len: len(shareddata.Composites.Docs()), + limitPushdown: allPushdown, }, "Big": { - limit: 1000, - len: len(shareddata.Composites.Docs()), - unsafeLimitPushdown: true, + limit: 1000, + len: len(shareddata.Composites.Docs()), + limitPushdown: allPushdown, }, "Zero": { - limit: 0, - len: len(shareddata.Composites.Docs()), - unsafeLimitPushdown: false, + limit: 0, + len: len(shareddata.Composites.Docs()), + limitPushdown: noPushdown, }, "IDFilter": { - filter: bson.D{{"_id", "array"}}, - limit: 3, - len: 1, - queryPushdown: allPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"_id", "array"}}, + limit: 3, + len: 1, + filterPushdown: allPushdown, + limitPushdown: noPushdown, }, "ValueFilter": { - filter: bson.D{{"v", 42}}, - sort: bson.D{{"_id", 1}}, - limit: 3, - len: 3, - queryPushdown: pgPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"v", 42}}, + sort: bson.D{{"_id", 1}}, + limit: 3, + len: 3, + filterPushdown: pgPushdown, + limitPushdown: noPushdown, }, "DotNotationFilter": { - filter: bson.D{{"v.foo", 42}}, - limit: 3, - len: 3, - queryPushdown: noPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"v.foo", 42}}, + limit: 3, + len: 3, + filterPushdown: noPushdown, + limitPushdown: noPushdown, }, "ObjectFilter": { - filter: bson.D{{"v", bson.D{{"foo", nil}}}}, - limit: 3, - len: 1, - queryPushdown: noPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"v", bson.D{{"foo", nil}}}}, + limit: 3, + len: 1, + filterPushdown: noPushdown, + limitPushdown: noPushdown, }, "Sort": { - sort: bson.D{{"_id", 1}}, - limit: 2, - len: 2, - queryPushdown: noPushdown, - unsafeLimitPushdown: true, + sort: bson.D{{"_id", 1}}, + limit: 2, + len: 2, + filterPushdown: noPushdown, + limitPushdown: allPushdown, }, "IDFilterSort": { - filter: bson.D{{"_id", "array"}}, - sort: bson.D{{"_id", 1}}, - limit: 3, - len: 1, - queryPushdown: allPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"_id", "array"}}, + sort: bson.D{{"_id", 1}}, + limit: 3, + len: 1, + filterPushdown: allPushdown, + limitPushdown: noPushdown, }, "ValueFilterSort": { - filter: bson.D{{"v", 42}}, - sort: bson.D{{"_id", 1}}, - limit: 3, - len: 3, - queryPushdown: pgPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"v", 42}}, + sort: bson.D{{"_id", 1}}, + limit: 3, + len: 3, + filterPushdown: pgPushdown, + limitPushdown: noPushdown, }, "DotNotationFilterSort": { - filter: bson.D{{"v.foo", 42}}, - sort: bson.D{{"_id", 1}}, - limit: 3, - len: 3, - queryPushdown: noPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"v.foo", 42}}, + sort: bson.D{{"_id", 1}}, + limit: 3, + len: 3, + filterPushdown: noPushdown, + limitPushdown: noPushdown, }, "ObjectFilterSort": { - filter: bson.D{{"v", bson.D{{"foo", nil}}}}, - sort: bson.D{{"_id", 1}}, - limit: 3, - len: 1, - queryPushdown: noPushdown, - unsafeLimitPushdown: false, + filter: bson.D{{"v", bson.D{{"foo", nil}}}}, + sort: bson.D{{"_id", 1}}, + limit: 3, + len: 1, + filterPushdown: noPushdown, + limitPushdown: noPushdown, }, "Skip": { - optSkip: pointer.ToInt64(1), - limit: 2, - len: 2, - unsafeLimitPushdown: false, + optSkip: pointer.ToInt64(1), + limit: 2, + len: 2, + limitPushdown: noPushdown, }, } { tc, name := tc, name @@ -996,23 +996,21 @@ func TestQueryCommandUnsafeLimitPushDown(t *testing.T) { var msg string if !setup.UnsafeSortPushdownEnabled() && tc.sort != nil { - tc.unsafeLimitPushdown = false + tc.limitPushdown = noPushdown msg = "Sort pushdown is disabled, but target resulted with limitPushdown" } - resultPushdown := tc.queryPushdown + doc := ConvertDocument(t, res) + limitPushdown, _ := doc.Get("limitPushdown") + assert.Equal(t, tc.limitPushdown.SortPushdownExpected(t, false), limitPushdown, msg) if setup.FilterPushdownDisabled() { - resultPushdown = noPushdown - msg = "Query pushdown is disabled, but target resulted with pushdown" + tc.filterPushdown = noPushdown + msg = "Filter pushdown is disabled, but target resulted with pushdown" } - doc := ConvertDocument(t, res) - unsafeLimitPushdown, _ := doc.Get("limitPushdown") - assert.Equal(t, tc.unsafeLimitPushdown, unsafeLimitPushdown, msg) - - queryPushdown, _ := ConvertDocument(t, res).Get("pushdown") - assert.Equal(t, resultPushdown.FilterPushdownExpected(t), queryPushdown, msg) + filterPushdown, _ := ConvertDocument(t, res).Get("filterPushdown") + assert.Equal(t, tc.filterPushdown.FilterPushdownExpected(t), filterPushdown, msg) }) t.Run("Find", func(t *testing.T) { diff --git a/internal/backends/collection.go b/internal/backends/collection.go index 3bb9924af87e..db0159f9ca6f 100644 --- a/internal/backends/collection.go +++ b/internal/backends/collection.go @@ -213,10 +213,10 @@ type ExplainParams struct { // ExplainResult represents the results of Collection.Explain method. type ExplainResult struct { - QueryPlanner *types.Document - QueryPushdown bool - UnsafeSortPushdown bool - UnsafeLimitPushdown bool + QueryPlanner *types.Document + FilterPushdown bool + SortPushdown bool + LimitPushdown bool } // Explain return a backend-specific execution plan for the given query. diff --git a/internal/backends/collection_test.go b/internal/backends/collection_test.go index af37e3dfc68a..273faecb59aa 100644 --- a/internal/backends/collection_test.go +++ b/internal/backends/collection_test.go @@ -93,7 +93,7 @@ func TestCollectionInsertAllQueryExplain(t *testing.T) { explainRes, err := cappedColl.Explain(ctx, new(backends.ExplainParams)) require.NoError(t, err) - assert.True(t, explainRes.UnsafeSortPushdown) + assert.True(t, explainRes.SortPushdown) }) t.Run("CappedCollectionOnlyRecordIDs", func(t *testing.T) { @@ -109,7 +109,7 @@ func TestCollectionInsertAllQueryExplain(t *testing.T) { explainRes, err := cappedColl.Explain(ctx, new(backends.ExplainParams)) require.NoError(t, err) - assert.True(t, explainRes.UnsafeSortPushdown) + assert.True(t, explainRes.SortPushdown) }) t.Run("CappedCollectionSortAsc", func(t *testing.T) { @@ -131,7 +131,7 @@ func TestCollectionInsertAllQueryExplain(t *testing.T) { explainRes, err := cappedColl.Explain(ctx, &backends.ExplainParams{Sort: &sort}) require.NoError(t, err) - assert.True(t, explainRes.UnsafeSortPushdown) + assert.True(t, explainRes.SortPushdown) }) t.Run("CappedCollectionSortDesc", func(t *testing.T) { @@ -153,7 +153,7 @@ func TestCollectionInsertAllQueryExplain(t *testing.T) { explainRes, err := cappedColl.Explain(ctx, &backends.ExplainParams{Sort: &sort}) require.NoError(t, err) - assert.True(t, explainRes.UnsafeSortPushdown) + assert.True(t, explainRes.SortPushdown) }) t.Run("CappedCollectionFilter", func(t *testing.T) { @@ -172,8 +172,8 @@ func TestCollectionInsertAllQueryExplain(t *testing.T) { explainRes, err := cappedColl.Explain(ctx, &backends.ExplainParams{Filter: filter}) require.NoError(t, err) - assert.True(t, explainRes.QueryPushdown) - assert.True(t, explainRes.UnsafeSortPushdown) + assert.True(t, explainRes.FilterPushdown) + assert.True(t, explainRes.SortPushdown) }) t.Run("NonCappedCollectionOnlyRecordID", func(t *testing.T) { @@ -191,7 +191,7 @@ func TestCollectionInsertAllQueryExplain(t *testing.T) { explainRes, err := coll.Explain(ctx, new(backends.ExplainParams)) require.NoError(t, err) - assert.False(t, explainRes.UnsafeSortPushdown) + assert.False(t, explainRes.SortPushdown) }) }) } diff --git a/internal/backends/postgresql/collection.go b/internal/backends/postgresql/collection.go index d52a12578ccc..afd9c8d85abd 100644 --- a/internal/backends/postgresql/collection.go +++ b/internal/backends/postgresql/collection.go @@ -320,19 +320,19 @@ func (c *collection) Explain(ctx context.Context, params *backends.ExplainParams return nil, lazyerrors.Error(err) } - res.QueryPushdown = where != "" + res.FilterPushdown = where != "" q += where sort, sortArgs := prepareOrderByClause(&placeholder, params.Sort, meta.Capped()) q += sort args = append(args, sortArgs...) - res.UnsafeSortPushdown = sort != "" + res.SortPushdown = sort != "" if params.Limit != 0 { q += fmt.Sprintf(` LIMIT %s`, placeholder.Next()) args = append(args, params.Limit) - res.UnsafeLimitPushdown = true + res.LimitPushdown = true } var b []byte diff --git a/internal/backends/sqlite/collection.go b/internal/backends/sqlite/collection.go index 5f2db548e5d5..c7f6402b65d7 100644 --- a/internal/backends/sqlite/collection.go +++ b/internal/backends/sqlite/collection.go @@ -257,7 +257,7 @@ func (c *collection) Explain(ctx context.Context, params *backends.ExplainParams selectClause := prepareSelectClause(meta.TableName, "", meta.Capped(), false) - var queryPushdown bool + var filterPushdown bool var whereClause string var args []any @@ -267,23 +267,23 @@ func (c *collection) Explain(ctx context.Context, params *backends.ExplainParams v, _ := params.Filter.Get("_id") switch v.(type) { case string, types.ObjectID: - queryPushdown = true + filterPushdown = true whereClause = fmt.Sprintf(` WHERE %s = ?`, metadata.IDColumn) args = []any{string(must.NotFail(sjson.MarshalSingleValue(v)))} } } orderByClause := prepareOrderByClause(params.Sort, meta.Capped()) - unsafeSortPushdown := orderByClause != "" + sortPushdown := orderByClause != "" q := `EXPLAIN QUERY PLAN ` + selectClause + whereClause + orderByClause - var unsafeLimitPushdown bool + var limitPushdown bool if params.Limit != 0 { q += ` LIMIT ?` args = append(args, params.Limit) - unsafeLimitPushdown = true + limitPushdown = true } rows, err := db.QueryContext(ctx, q, args...) @@ -316,10 +316,10 @@ func (c *collection) Explain(ctx context.Context, params *backends.ExplainParams } return &backends.ExplainResult{ - QueryPlanner: must.NotFail(types.NewDocument("Plan", queryPlan)), - QueryPushdown: queryPushdown, - UnsafeSortPushdown: unsafeSortPushdown, - UnsafeLimitPushdown: unsafeLimitPushdown, + QueryPlanner: must.NotFail(types.NewDocument("Plan", queryPlan)), + FilterPushdown: filterPushdown, + SortPushdown: sortPushdown, + LimitPushdown: limitPushdown, }, nil } diff --git a/internal/backends/sqlite/collection_test.go b/internal/backends/sqlite/collection_test.go index 741c6671b941..3a1e482930c9 100644 --- a/internal/backends/sqlite/collection_test.go +++ b/internal/backends/sqlite/collection_test.go @@ -82,6 +82,6 @@ func TestCappedCollectionInsertAllQueryExplain(t *testing.T) { explainRes, err := cappedColl.Explain(ctx, &backends.ExplainParams{Sort: &sort}) require.NoError(t, err) - assert.False(t, explainRes.UnsafeSortPushdown) + assert.False(t, explainRes.SortPushdown) }) } diff --git a/internal/handlers/sqlite/msg_aggregate.go b/internal/handlers/sqlite/msg_aggregate.go index 3b28d385fa25..4c84d02b602a 100644 --- a/internal/handlers/sqlite/msg_aggregate.go +++ b/internal/handlers/sqlite/msg_aggregate.go @@ -277,6 +277,7 @@ func (h *Handler) MsgAggregate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs order, err = common.GetSortType(k, v) if err != nil { + closer.Close() return nil, err } diff --git a/internal/handlers/sqlite/msg_explain.go b/internal/handlers/sqlite/msg_explain.go index 775b365338c9..dd0ab4b979b3 100644 --- a/internal/handlers/sqlite/msg_explain.go +++ b/internal/handlers/sqlite/msg_explain.go @@ -137,9 +137,9 @@ func (h *Handler) MsgExplain(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, // our extensions // TODO https://github.com/FerretDB/FerretDB/issues/3235 - "pushdown", res.QueryPushdown, - "sortingPushdown", res.UnsafeSortPushdown, - "limitPushdown", res.UnsafeLimitPushdown, + "filterPushdown", res.FilterPushdown, + "sortPushdown", res.SortPushdown, + "limitPushdown", res.LimitPushdown, "ok", float64(1), ))},