From 1b97b267492c06ab6849bd69b585f12583e563c4 Mon Sep 17 00:00:00 2001 From: noisersup Date: Thu, 23 Nov 2023 11:41:34 +0100 Subject: [PATCH 1/5] init --- cmd/ferretdb/main.go | 10 ++++------ integration/pushdown.go | 8 +------- integration/query_test.go | 11 +++-------- integration/setup/listener.go | 5 ++--- integration/setup/setup.go | 3 +-- integration/setup/test_helpers.go | 5 ----- internal/handlers/registry/hana.go | 5 ++--- internal/handlers/registry/registry.go | 5 ++--- internal/handlers/registry/sqlite.go | 5 ++--- 9 files changed, 17 insertions(+), 40 deletions(-) diff --git a/cmd/ferretdb/main.go b/cmd/ferretdb/main.go index 67ef971cd748..6e7b994ea780 100644 --- a/cmd/ferretdb/main.go +++ b/cmd/ferretdb/main.go @@ -86,9 +86,8 @@ var cli struct { Test struct { RecordsDir string `default:"" help:"Testing: directory for record files."` - DisableFilterPushdown bool `default:"false" help:"Experimental: disable filter pushdown."` - EnableUnsafeSortPushdown bool `default:"false" help:"Experimental: enable unsafe sort pushdown."` - EnableOplog bool `default:"false" help:"Experimental: enable capped collections, tailable cursors and OpLog." hidden:""` + DisableFilterPushdown bool `default:"false" help:"Experimental: disable filter pushdown."` + EnableOplog bool `default:"false" help:"Experimental: enable capped collections, tailable cursors and OpLog." hidden:""` //nolint:lll // for readability Telemetry struct { @@ -390,9 +389,8 @@ func run() { MySQLURL: mySQLFlags.MySQLURL, TestOpts: registry.TestOpts{ - DisableFilterPushdown: cli.Test.DisableFilterPushdown, - EnableUnsafeSortPushdown: cli.Test.EnableUnsafeSortPushdown, - EnableOplog: cli.Test.EnableOplog, + DisableFilterPushdown: cli.Test.DisableFilterPushdown, + EnableOplog: cli.Test.EnableOplog, }, }) if err != nil { diff --git a/integration/pushdown.go b/integration/pushdown.go index 21a4cb2492a5..767aef30b226 100644 --- a/integration/pushdown.go +++ b/integration/pushdown.go @@ -55,13 +55,7 @@ func (res resultPushdown) FilterPushdownExpected(t testtb.TB) bool { } // SortPushdownExpected returns true if sort pushdown is expected for currently running backend. -// It checks if pushdown is enabled by flag. -// For capped collection, pushdown for recordID is done even if pushdown is not enabled by flag. -func (res resultPushdown) SortPushdownExpected(t testtb.TB, cappedCollection bool) bool { - if !setup.UnsafeSortPushdownEnabled() && cappedCollection { - res = allPushdown - } - +func (res resultPushdown) SortPushdownExpected(t testtb.TB) bool { return res.pushdownExpected(t) } diff --git a/integration/query_test.go b/integration/query_test.go index e763ed820c31..7c45ba4c21f6 100644 --- a/integration/query_test.go +++ b/integration/query_test.go @@ -993,16 +993,11 @@ func TestQueryCommandLimitPushDown(t *testing.T) { assert.NoError(t, err) - var msg string - - if !setup.UnsafeSortPushdownEnabled() && tc.sort != nil { - tc.limitPushdown = noPushdown - msg = "Sort pushdown is disabled, but target resulted with limitPushdown" - } - doc := ConvertDocument(t, res) limitPushdown, _ := doc.Get("limitPushdown") - assert.Equal(t, tc.limitPushdown.SortPushdownExpected(t, false), limitPushdown, msg) + assert.Equal(t, tc.limitPushdown.SortPushdownExpected(t), limitPushdown) + + var msg string if setup.FilterPushdownDisabled() { tc.filterPushdown = noPushdown diff --git a/integration/setup/listener.go b/integration/setup/listener.go index dddaacda86bf..1471eee43b00 100644 --- a/integration/setup/listener.go +++ b/integration/setup/listener.go @@ -159,9 +159,8 @@ func setupListener(tb testtb.TB, ctx context.Context, logger *zap.Logger) string HANAURL: *hanaURLF, TestOpts: registry.TestOpts{ - DisableFilterPushdown: *disableFilterPushdownF, - EnableUnsafeSortPushdown: *enableUnsafeSortPushdownF, - EnableOplog: true, + DisableFilterPushdown: *disableFilterPushdownF, + EnableOplog: true, }, } h, closeBackend, err := registry.NewHandler(handler, handlerOpts) diff --git a/integration/setup/setup.go b/integration/setup/setup.go index 001ccbd5fff3..a938053ff27d 100644 --- a/integration/setup/setup.go +++ b/integration/setup/setup.go @@ -59,8 +59,7 @@ var ( debugSetupF = flag.Bool("debug-setup", false, "enable debug logs for tests setup") logLevelF = zap.LevelFlag("log-level", zap.DebugLevel, "log level for tests") - disableFilterPushdownF = flag.Bool("disable-filter-pushdown", false, "disable filter pushdown") - enableUnsafeSortPushdownF = flag.Bool("enable-unsafe-sort-pushdown", false, "enable unsafe sort pushdown") + disableFilterPushdownF = flag.Bool("disable-filter-pushdown", false, "disable filter pushdown") ) // Other globals. diff --git a/integration/setup/test_helpers.go b/integration/setup/test_helpers.go index 8c25202fef4c..32e294fa5317 100644 --- a/integration/setup/test_helpers.go +++ b/integration/setup/test_helpers.go @@ -85,8 +85,3 @@ func SkipForMongoDB(tb testtb.TB, reason string) { func FilterPushdownDisabled() bool { return *disableFilterPushdownF } - -// UnsafeSortPushdownEnabled returns true if unsafe sort pushdown is enabled. -func UnsafeSortPushdownEnabled() bool { - return *enableUnsafeSortPushdownF -} diff --git a/internal/handlers/registry/hana.go b/internal/handlers/registry/hana.go index 773c32fd7949..db516f64f28c 100644 --- a/internal/handlers/registry/hana.go +++ b/internal/handlers/registry/hana.go @@ -43,9 +43,8 @@ func init() { ConnMetrics: opts.ConnMetrics, StateProvider: opts.StateProvider, - DisableFilterPushdown: opts.DisableFilterPushdown, - EnableUnsafeSortPushdown: opts.EnableUnsafeSortPushdown, - EnableOplog: opts.EnableOplog, + DisableFilterPushdown: opts.DisableFilterPushdown, + EnableOplog: opts.EnableOplog, } h, err := handler.New(handlerOpts) diff --git a/internal/handlers/registry/registry.go b/internal/handlers/registry/registry.go index 8b70efd3da9e..6abc91689906 100644 --- a/internal/handlers/registry/registry.go +++ b/internal/handlers/registry/registry.go @@ -61,9 +61,8 @@ type NewHandlerOpts struct { // TestOpts represents experimental configuration options. type TestOpts struct { - DisableFilterPushdown bool - EnableUnsafeSortPushdown bool - EnableOplog bool + DisableFilterPushdown bool + EnableOplog bool } // NewHandler constructs a new handler. diff --git a/internal/handlers/registry/sqlite.go b/internal/handlers/registry/sqlite.go index 77a7cfe2348e..75382567e3aa 100644 --- a/internal/handlers/registry/sqlite.go +++ b/internal/handlers/registry/sqlite.go @@ -39,9 +39,8 @@ func init() { ConnMetrics: opts.ConnMetrics, StateProvider: opts.StateProvider, - DisableFilterPushdown: opts.DisableFilterPushdown, - EnableUnsafeSortPushdown: opts.EnableUnsafeSortPushdown, - EnableOplog: opts.EnableOplog, + DisableFilterPushdown: opts.DisableFilterPushdown, + EnableOplog: opts.EnableOplog, } h, err := handler.New(handlerOpts) From 08bc1f8f361d11b9d19f55b4dd62153bd51dc02f Mon Sep 17 00:00:00 2001 From: noisersup Date: Thu, 23 Nov 2023 12:56:02 +0100 Subject: [PATCH 2/5] wip --- internal/handlers/registry/mysql.go | 5 ++- internal/handlers/registry/postgresql.go | 5 ++- internal/handlers/sqlite/msg_aggregate.go | 32 +++++++++---------- internal/handlers/sqlite/msg_explain.go | 39 ++++++++++------------- internal/handlers/sqlite/msg_find.go | 35 ++++++++++---------- internal/handlers/sqlite/sqlite.go | 5 ++- 6 files changed, 56 insertions(+), 65 deletions(-) diff --git a/internal/handlers/registry/mysql.go b/internal/handlers/registry/mysql.go index 8005dd647fa0..c980fff5e59d 100644 --- a/internal/handlers/registry/mysql.go +++ b/internal/handlers/registry/mysql.go @@ -39,9 +39,8 @@ func init() { ConnMetrics: opts.ConnMetrics, StateProvider: opts.StateProvider, - DisableFilterPushdown: opts.DisableFilterPushdown, - EnableUnsafeSortPushdown: opts.EnableUnsafeSortPushdown, - EnableOplog: opts.EnableOplog, + DisableFilterPushdown: opts.DisableFilterPushdown, + EnableOplog: opts.EnableOplog, } h, err := handler.New(handlerOpts) diff --git a/internal/handlers/registry/postgresql.go b/internal/handlers/registry/postgresql.go index c44b5ef56d99..6e4575aa301d 100644 --- a/internal/handlers/registry/postgresql.go +++ b/internal/handlers/registry/postgresql.go @@ -39,9 +39,8 @@ func init() { ConnMetrics: opts.ConnMetrics, StateProvider: opts.StateProvider, - DisableFilterPushdown: opts.DisableFilterPushdown, - EnableUnsafeSortPushdown: opts.EnableUnsafeSortPushdown, - EnableOplog: opts.EnableOplog, + DisableFilterPushdown: opts.DisableFilterPushdown, + EnableOplog: opts.EnableOplog, } h, err := handler.New(handlerOpts) diff --git a/internal/handlers/sqlite/msg_aggregate.go b/internal/handlers/sqlite/msg_aggregate.go index 4c84d02b602a..4a2596830780 100644 --- a/internal/handlers/sqlite/msg_aggregate.go +++ b/internal/handlers/sqlite/msg_aggregate.go @@ -259,7 +259,7 @@ func (h *Handler) MsgAggregate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs var iter iterator.Interface[struct{}, *types.Document] if len(collStatsDocuments) == len(stagesDocuments) { - filter, sort := aggregations.GetPushdownQuery(aggregationStages) + filter, _ := aggregations.GetPushdownQuery(aggregationStages) // only documents stages or no stages - fetch documents from the DB and apply stages to them qp := new(backends.QueryParams) @@ -268,24 +268,24 @@ func (h *Handler) MsgAggregate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs qp.Filter = filter } - // Skip sorting if there are more than one sort parameters - if h.EnableUnsafeSortPushdown && sort.Len() == 1 { - var order types.SortType + //// Skip sorting if there are more than one sort parameters + //if h.EnableUnsafeSortPushdown && sort.Len() == 1 { + // var order types.SortType - k := sort.Keys()[0] - v := sort.Values()[0] + // k := sort.Keys()[0] + // v := sort.Values()[0] - order, err = common.GetSortType(k, v) - if err != nil { - closer.Close() - return nil, err - } + // order, err = common.GetSortType(k, v) + // if err != nil { + // closer.Close() + // return nil, err + // } - qp.Sort = &backends.SortField{ - Key: k, - Descending: order == types.Descending, - } - } + // qp.Sort = &backends.SortField{ + // Key: k, + // Descending: order == types.Descending, + // } + //} iter, err = processStagesDocuments(ctx, closer, &stagesDocumentsParams{c, qp, stagesDocuments}) } else { diff --git a/internal/handlers/sqlite/msg_explain.go b/internal/handlers/sqlite/msg_explain.go index d39cc39fb259..d3cda05e7be0 100644 --- a/internal/handlers/sqlite/msg_explain.go +++ b/internal/handlers/sqlite/msg_explain.go @@ -87,31 +87,30 @@ func (h *Handler) MsgExplain(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, qp.Filter, params.Sort = aggregations.GetPushdownQuery(params.StagesDocs) } - // Skip sorting if there are more than one sort parameters - // TODO https://github.com/FerretDB/FerretDB/issues/3742 - if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 { - var order types.SortType + //// Skip sorting if there are more than one sort parameters + //// TODO https://github.com/FerretDB/FerretDB/issues/3742 + //if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 { + // var order types.SortType - k := params.Sort.Keys()[0] - v := params.Sort.Values()[0] + // k := params.Sort.Keys()[0] + // v := params.Sort.Values()[0] - order, err = common.GetSortType(k, v) - if err != nil { - return nil, err - } + // order, err = common.GetSortType(k, v) + // if err != nil { + // return nil, err + // } - qp.Sort = &backends.SortField{ - Key: k, - Descending: order == types.Descending, - } - } + // qp.Sort = &backends.SortField{ + // Key: k, + // Descending: order == types.Descending, + // } + //} // Limit pushdown is not applied if: // - `filter` is set, it must fetch all documents to filter them in memory; - // - `sort` is set but `UnsafeSortPushdown` is not set, it must fetch all documents - // and sort them in memory; + // - `sort` is set, it must fetch all documents and sort them in memory; // - `skip` is non-zero value, skip pushdown is not supported yet. - if params.Filter.Len() == 0 && (params.Sort.Len() == 0 || h.EnableUnsafeSortPushdown) && params.Skip == 0 { + if params.Filter.Len() == 0 && params.Sort.Len() == 0 && params.Skip == 0 { qp.Limit = params.Limit } @@ -119,10 +118,6 @@ func (h *Handler) MsgExplain(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, qp.Filter = nil } - if !h.EnableUnsafeSortPushdown { - qp.Sort = nil - } - res, err := coll.Explain(ctx, &qp) if err != nil { return nil, lazyerrors.Error(err) diff --git a/internal/handlers/sqlite/msg_find.go b/internal/handlers/sqlite/msg_find.go index ac21d4b36e3b..dc0c0af7a603 100644 --- a/internal/handlers/sqlite/msg_find.go +++ b/internal/handlers/sqlite/msg_find.go @@ -112,31 +112,30 @@ func (h *Handler) MsgFind(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, er qp.Filter = params.Filter } - // Skip sorting if there are more than one sort parameters - // TODO https://github.com/FerretDB/FerretDB/issues/3742 - if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 { - var order types.SortType + //// Skip sorting if there are more than one sort parameters + //// TODO https://github.com/FerretDB/FerretDB/issues/3742 + //if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 { + // var order types.SortType - k := params.Sort.Keys()[0] - v := params.Sort.Values()[0] + // k := params.Sort.Keys()[0] + // v := params.Sort.Values()[0] - order, err = common.GetSortType(k, v) - if err != nil { - return nil, err - } + // order, err = common.GetSortType(k, v) + // if err != nil { + // return nil, err + // } - qp.Sort = &backends.SortField{ - Key: k, - Descending: order == types.Descending, - } - } + // qp.Sort = &backends.SortField{ + // Key: k, + // Descending: order == types.Descending, + // } + //} // Limit pushdown is not applied if: // - `filter` is set, it must fetch all documents to filter them in memory; - // - `sort` is set but `UnsafeSortPushdown` is not set, it must fetch all documents - // and sort them in memory; + // - `sort` is set, it must fetch all documents and sort them in memory; // - `skip` is non-zero value, skip pushdown is not supported yet. - if params.Filter.Len() == 0 && (params.Sort.Len() == 0 || h.EnableUnsafeSortPushdown) && params.Skip == 0 { + if params.Filter.Len() == 0 && params.Sort.Len() == 0 && params.Skip == 0 { qp.Limit = params.Limit } diff --git a/internal/handlers/sqlite/sqlite.go b/internal/handlers/sqlite/sqlite.go index 80a2984672e7..f74d13432ecd 100644 --- a/internal/handlers/sqlite/sqlite.go +++ b/internal/handlers/sqlite/sqlite.go @@ -49,9 +49,8 @@ type NewOpts struct { StateProvider *state.Provider // test options - DisableFilterPushdown bool - EnableUnsafeSortPushdown bool - EnableOplog bool + DisableFilterPushdown bool + EnableOplog bool } // New returns a new handler. From 7cd1e0c194572ef7976998ee30369d9f79738628 Mon Sep 17 00:00:00 2001 From: noisersup Date: Thu, 23 Nov 2023 12:59:30 +0100 Subject: [PATCH 3/5] wip --- integration/query_compat_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/query_compat_test.go b/integration/query_compat_test.go index 93975fb693b5..e0e0750186b1 100644 --- a/integration/query_compat_test.go +++ b/integration/query_compat_test.go @@ -289,7 +289,7 @@ func TestQueryCappedCollectionCompat(t *testing.T) { doc := ConvertDocument(t, explainRes) sortPushdown, _ := doc.Get("sortPushdown") - assert.Equal(t, tc.sortPushdown.SortPushdownExpected(t, true), sortPushdown) + assert.Equal(t, tc.sortPushdown.SortPushdownExpected(t), sortPushdown) findOpts := options.Find() if tc.sort != nil { From ea3092a33837e5f6e4dc7613392acf34ec32d3fe Mon Sep 17 00:00:00 2001 From: noisersup Date: Thu, 23 Nov 2023 13:14:01 +0100 Subject: [PATCH 4/5] wip --- integration/query_compat_test.go | 4 ++-- integration/query_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/integration/query_compat_test.go b/integration/query_compat_test.go index e0e0750186b1..bc2c158763bb 100644 --- a/integration/query_compat_test.go +++ b/integration/query_compat_test.go @@ -253,12 +253,12 @@ func TestQueryCappedCollectionCompat(t *testing.T) { }, "Sort": { sort: bson.D{{"_id", int32(-1)}}, - sortPushdown: pgPushdown, + sortPushdown: allPushdown, }, "FilterSort": { filter: bson.D{{"v", int32(42)}}, sort: bson.D{{"_id", int32(-1)}}, - sortPushdown: pgPushdown, + sortPushdown: allPushdown, }, "MultipleSortFields": { sort: bson.D{{"v", 1}, {"_id", int32(-1)}}, diff --git a/integration/query_test.go b/integration/query_test.go index 7c45ba4c21f6..5fad00521df3 100644 --- a/integration/query_test.go +++ b/integration/query_test.go @@ -906,7 +906,7 @@ func TestQueryCommandLimitPushDown(t *testing.T) { limit: 2, len: 2, filterPushdown: noPushdown, - limitPushdown: allPushdown, + limitPushdown: noPushdown, }, "IDFilterSort": { filter: bson.D{{"_id", "array"}}, From cae48b0c489fff5bf54bb715f138316645c6a4a5 Mon Sep 17 00:00:00 2001 From: noisersup Date: Thu, 23 Nov 2023 13:25:10 +0100 Subject: [PATCH 5/5] rm comments --- internal/handlers/sqlite/msg_aggregate.go | 19 ------------------- internal/handlers/sqlite/msg_explain.go | 19 ------------------- internal/handlers/sqlite/msg_find.go | 19 ------------------- 3 files changed, 57 deletions(-) diff --git a/internal/handlers/sqlite/msg_aggregate.go b/internal/handlers/sqlite/msg_aggregate.go index 4a2596830780..6103440b944b 100644 --- a/internal/handlers/sqlite/msg_aggregate.go +++ b/internal/handlers/sqlite/msg_aggregate.go @@ -268,25 +268,6 @@ func (h *Handler) MsgAggregate(ctx context.Context, msg *wire.OpMsg) (*wire.OpMs qp.Filter = filter } - //// Skip sorting if there are more than one sort parameters - //if h.EnableUnsafeSortPushdown && sort.Len() == 1 { - // var order types.SortType - - // k := sort.Keys()[0] - // v := sort.Values()[0] - - // order, err = common.GetSortType(k, v) - // if err != nil { - // closer.Close() - // return nil, err - // } - - // qp.Sort = &backends.SortField{ - // Key: k, - // Descending: order == types.Descending, - // } - //} - iter, err = processStagesDocuments(ctx, closer, &stagesDocumentsParams{c, qp, stagesDocuments}) } else { // TODO https://github.com/FerretDB/FerretDB/issues/2423 diff --git a/internal/handlers/sqlite/msg_explain.go b/internal/handlers/sqlite/msg_explain.go index d3cda05e7be0..fac1189c1225 100644 --- a/internal/handlers/sqlite/msg_explain.go +++ b/internal/handlers/sqlite/msg_explain.go @@ -87,25 +87,6 @@ func (h *Handler) MsgExplain(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, qp.Filter, params.Sort = aggregations.GetPushdownQuery(params.StagesDocs) } - //// Skip sorting if there are more than one sort parameters - //// TODO https://github.com/FerretDB/FerretDB/issues/3742 - //if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 { - // var order types.SortType - - // k := params.Sort.Keys()[0] - // v := params.Sort.Values()[0] - - // order, err = common.GetSortType(k, v) - // if err != nil { - // return nil, err - // } - - // qp.Sort = &backends.SortField{ - // Key: k, - // Descending: order == types.Descending, - // } - //} - // Limit pushdown is not applied if: // - `filter` is set, it must fetch all documents to filter them in memory; // - `sort` is set, it must fetch all documents and sort them in memory; diff --git a/internal/handlers/sqlite/msg_find.go b/internal/handlers/sqlite/msg_find.go index dc0c0af7a603..a2ccfd9b9b63 100644 --- a/internal/handlers/sqlite/msg_find.go +++ b/internal/handlers/sqlite/msg_find.go @@ -112,25 +112,6 @@ func (h *Handler) MsgFind(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg, er qp.Filter = params.Filter } - //// Skip sorting if there are more than one sort parameters - //// TODO https://github.com/FerretDB/FerretDB/issues/3742 - //if h.EnableUnsafeSortPushdown && params.Sort.Len() == 1 { - // var order types.SortType - - // k := params.Sort.Keys()[0] - // v := params.Sort.Values()[0] - - // order, err = common.GetSortType(k, v) - // if err != nil { - // return nil, err - // } - - // qp.Sort = &backends.SortField{ - // Key: k, - // Descending: order == types.Descending, - // } - //} - // Limit pushdown is not applied if: // - `filter` is set, it must fetch all documents to filter them in memory; // - `sort` is set, it must fetch all documents and sort them in memory;