Use single flag to disable all pushdowns (#3793)

FerretDB · Dec 6, 2023 · 504c567 · 504c567
1 parent 9823d75
commit 504c567
Show file tree

Hide file tree

Showing 37 changed files with 268 additions and 210 deletions.
diff --git a/.github/workflows/_integration.yml b/.github/workflows/_integration.yml
@@ -15,7 +15,7 @@ on:
       coveralls:
         required: true
         type: boolean
-      disable_filter_pushdown:
+      disable_pushdown:
         required: false
         type: boolean
         default: false
@@ -88,13 +88,13 @@ jobs:
       - name: >
           Run ${{ inputs.task }} tests
           (${{ inputs.shard_index }}/${{ inputs.shard_total }},
-          filter=${{ !inputs.disable_filter_pushdown }})
+          pushdown=${{ !inputs.disable_pushdown }})
         run: >
           bin/task test-integration-${{ inputs.task }}
           SHARD_INDEX=${{ inputs.shard_index }}
           SHARD_TOTAL=${{ inputs.shard_total }}
           TEST_TIMEOUT=15m
-          DISABLE_FILTER_PUSHDOWN=${{ inputs.disable_filter_pushdown }}
+          DISABLE_PUSHDOWN=${{ inputs.disable_pushdown }}
         env:
           GOFLAGS: ${{ runner.debug == '1' && '-v' || '' }}
           FERRETDB_HANA_URL: ${{ secrets.FERRETDB_HANA_URL }}
@@ -112,7 +112,7 @@ jobs:
         with:
           token: 22159d7c-856d-4fe9-8fdb-5d9ecff35514
           files: ./integration/integration-${{ inputs.task }}.txt
-          flags: integration,${{ inputs.task }}-${{ inputs.shard_index }},filter-${{ !inputs.disable_filter_pushdown }}
+          flags: integration,${{ inputs.task }}-${{ inputs.shard_index }},filter-${{ !inputs.disable_pushdown }}
           fail_ci_if_error: true
           verbose: true
 
@@ -121,7 +121,7 @@ jobs:
         uses: coverallsapp/github-action@v2
         with:
           file: ./integration/integration-${{ inputs.task }}.txt
-          flag-name: integration-${{ inputs.task }}-${{ inputs.shard_index }}-filter-${{ !inputs.disable_filter_pushdown }}
+          flag-name: integration-${{ inputs.task }}-${{ inputs.shard_index }}-filter-${{ !inputs.disable_pushdown }}
           parallel: true
 
       # we don't want them on CI

diff --git a/.github/workflows/go-extra.yml b/.github/workflows/go-extra.yml
@@ -19,11 +19,11 @@ jobs:
     # job name must be unique; make it unique and nice
     name: >
       ${{ matrix.task }} ${{ matrix.shard_index }}/${{ matrix.shard_total }}
-      (filter=${{ matrix.disable_filter_pushdown }})
+      (pushdown=${{ matrix.disable_pushdown }})
 
     # To avoid conflict with go.yml.
     concurrency:
-      group: ${{ github.workflow }}-integration-${{ matrix.task }}-${{ matrix.shard_index }}-${{ matrix.disable_filter_pushdown }}-${{ github.head_ref || github.ref_name }}
+      group: ${{ github.workflow }}-integration-${{ matrix.task }}-${{ matrix.shard_index }}-${{ matrix.disable_pushdown }}-${{ github.head_ref || github.ref_name }}
       cancel-in-progress: true
 
     strategy:
@@ -32,7 +32,7 @@ jobs:
         task: [postgresql, sqlite, mysql]
         shard_index: [1, 2, 3]
         shard_total: [3]
-        disable_filter_pushdown: [false, true]
+        disable_pushdown: [false, true]
 
     # Do not submit to coveralls because it can't handle parallel workflows:
     # https://github.com/lemurheavy/coveralls-public/issues/1636#issuecomment-1529460515
@@ -41,5 +41,5 @@ jobs:
       task: ${{ matrix.task }}
       shard_index: ${{ matrix.shard_index }}
       shard_total: ${{ matrix.shard_total }}
-      disable_filter_pushdown: ${{ matrix.disable_filter_pushdown }}
+      disable_pushdown: ${{ matrix.disable_pushdown }}
       coveralls: false
diff --git a/Taskfile.yml b/Taskfile.yml
@@ -8,7 +8,7 @@ env:
 vars:
   SHARD_INDEX: 1
   SHARD_TOTAL: 1
-  DISABLE_FILTER_PUSHDOWN: false
+  DISABLE_PUSHDOWN: false
   TEST_RUN: ""
   TEST_TIMEOUT: 35m
   BENCH_TIME: 5s
@@ -216,7 +216,7 @@ tasks:
         -target-tls
         -postgresql-url='postgres://username@127.0.0.1:5432/ferretdb?search_path='
         -compat-url='mongodb://username:password@127.0.0.1:47018/?tls=true&tlsCertificateKeyFile=../build/certs/client.pem&tlsCaFile=../build/certs/rootCA-cert.pem'
-        -disable-filter-pushdown={{.DISABLE_FILTER_PUSHDOWN}}
+        -disable-pushdown={{.DISABLE_PUSHDOWN}}
 
   test-integration-sqlite:
     desc: "Run integration tests for `sqlite` backend"
@@ -241,7 +241,7 @@ tasks:
         -sqlite-url=file:../tmp/sqlite-tests/
         -target-tls
         -compat-url='mongodb://username:password@127.0.0.1:47018/?tls=true&tlsCertificateKeyFile=../build/certs/client.pem&tlsCaFile=../build/certs/rootCA-cert.pem'
-        -disable-filter-pushdown={{.DISABLE_FILTER_PUSHDOWN}}
+        -disable-pushdown={{.DISABLE_PUSHDOWN}}
 
   test-integration-mysql:
     desc: "Run integration tests for `mysql` handler"
@@ -266,7 +266,7 @@ tasks:
         -target-tls
         -mysql-url='mysql://username:password@127.0.0.1:3306/ferretdb'
         -compat-url='mongodb://username:password@127.0.0.1:47018/?tls=true&tlsCertificateKeyFile=../build/certs/client.pem&tlsCaFile=../build/certs/rootCA-cert.pem'
-        -disable-filter-pushdown={{.DISABLE_FILTER_PUSHDOWN}}
+        -disable-pushdown={{.DISABLE_PUSHDOWN}}
 
   test-integration-hana:
     desc: "Run integration tests for `hana` handler"
@@ -291,7 +291,7 @@ tasks:
         -target-tls
         -hana-url=$FERRETDB_HANA_URL
         -compat-url='mongodb://username:password@127.0.0.1:47018/?tls=true&tlsCertificateKeyFile=../build/certs/client.pem&tlsCaFile=../build/certs/rootCA-cert.pem'
-        -disable-filter-pushdown={{.DISABLE_FILTER_PUSHDOWN}}
+        -disable-pushdown={{.DISABLE_PUSHDOWN}}
 
   test-integration-mongodb:
     desc: "Run integration tests for MongoDB"

diff --git a/cmd/ferretdb/main.go b/cmd/ferretdb/main.go
@@ -92,9 +92,9 @@ var cli struct {
 	Test struct {
 		RecordsDir string `default:"" help:"Testing: directory for record files."`
 
-		DisableFilterPushdown bool `default:"false" help:"Experimental: disable filter pushdown."`
-		EnableOplog           bool `default:"false" help:"Experimental: enable capped collections, tailable cursors and OpLog." hidden:""`
-		EnableNewAuth         bool `default:"false" help:"Experimental: enable new authentication."                             hidden:""`
+		DisablePushdown bool `default:"false" help:"Experimental: disable pushdown."`
+		EnableOplog     bool `default:"false" help:"Experimental: enable capped collections, tailable cursors and OpLog." hidden:""`
+		EnableNewAuth   bool `default:"false" help:"Experimental: enable new authentication."                             hidden:""`
 
 		//nolint:lll // for readability
 		Telemetry struct {
@@ -396,9 +396,9 @@ func run() {
 		MySQLURL: mySQLFlags.MySQLURL,
 
 		TestOpts: registry.TestOpts{
-			DisableFilterPushdown: cli.Test.DisableFilterPushdown,
-			EnableOplog:           cli.Test.EnableOplog,
-			EnableNewAuth:         cli.Test.EnableNewAuth,
+			DisablePushdown: cli.Test.DisablePushdown,
+			EnableOplog:     cli.Test.EnableOplog,
+			EnableNewAuth:   cli.Test.EnableNewAuth,
 		},
 	})
 	if err != nil {

diff --git a/integration/Taskfile.yml b/integration/Taskfile.yml
@@ -80,7 +80,7 @@ tasks:
         -bench-docs={{.BENCH_DOCS}}
         -target-backend=ferretdb-postgresql
         -postgresql-url=postgres://username@127.0.0.1:5432/ferretdb
-        -disable-filter-pushdown
+        -disable-pushdown
         | tee new-postgresql.txt
       - ../bin/benchstat{{exeExt}} old-postgresql.txt new-postgresql.txt
 
@@ -107,7 +107,7 @@ tasks:
         -bench-docs={{.BENCH_DOCS}}
         -target-backend=ferretdb-sqlite
         -sqlite-url=file:../tmp/sqlite-tests/
-        -disable-filter-pushdown
+        -disable-pushdown
         | tee new-sqlite.txt
       - ../bin/benchstat{{exeExt}} old-sqlite.txt new-sqlite.txt
 
@@ -134,7 +134,7 @@ tasks:
         -bench-docs={{.BENCH_DOCS}}
         -target-backend=ferretdb-mysql
         -mysql-url=mysql://username:password@127.0.0.1:3306/ferretdb
-        -disable-filter-pushdown
+        -disable-pushdown
         | tee new-sqlite.txt
       - ../bin/benchstat{{exeExt}} old-sqlite.txt new-sqlite.txt
 

diff --git a/integration/aggregate_documents_compat_test.go b/integration/aggregate_documents_compat_test.go
@@ -145,14 +145,14 @@ func testAggregateStagesCompatWithProviders(t *testing.T, providers shareddata.P
 
 					var msg string
 					// TODO https://github.com/FerretDB/FerretDB/issues/3386
-					if setup.FilterPushdownDisabled() {
+					if setup.PushdownDisabled() {
 						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)
+					assert.Equal(t, resPushdown.PushdownExpected(t), pushdown, msg)
 				})
 			}
 

diff --git a/integration/pushdown.go b/integration/pushdown.go
@@ -44,21 +44,16 @@ func init() {
 	must.BeTrue(allPushdown == 0b11111111)
 }
 
-// FilterPushdownExpected returns true if the filter pushdown is expected for currently running backend.
-// It checks if filter pushdown is disabled by flag.
-func (res resultPushdown) FilterPushdownExpected(t testtb.TB) bool {
-	if setup.FilterPushdownDisabled() {
+// PushdownExpected returns true if pushdown is expected for currently running backend.
+// It checks if pushdown is disabled by flag.
+func (res resultPushdown) PushdownExpected(t testtb.TB) bool {
+	if setup.PushdownDisabled() {
 		res = noPushdown
 	}
 
 	return res.pushdownExpected(t)
 }
 
-// SortPushdownExpected returns true if sort pushdown is expected for currently running backend.
-func (res resultPushdown) SortPushdownExpected(t testtb.TB) bool {
-	return res.pushdownExpected(t)
-}
-
 // pushdownExpected returns true if pushdown is expected for currently running backend.
 func (res resultPushdown) pushdownExpected(t testtb.TB) bool {
 	switch {

diff --git a/integration/query_command_compat_test.go b/integration/query_command_compat_test.go
@@ -102,14 +102,14 @@ func testQueryCommandCompat(t *testing.T, testCases map[string]queryCommandCompa
 					require.NoError(t, targetCollection.Database().RunCommand(ctx, explainQuery).Decode(&explainRes))
 
 					var msg string
-					if setup.FilterPushdownDisabled() {
+					if setup.PushdownDisabled() {
 						tc.filterPushdown = noPushdown
 						msg = "Filter pushdown is disabled, but target resulted with pushdown"
 					}
 
 					doc := ConvertDocument(t, explainRes)
 					pushdown, _ := doc.Get("filterPushdown")
-					assert.Equal(t, tc.filterPushdown.FilterPushdownExpected(t), pushdown, msg)
+					assert.Equal(t, tc.filterPushdown.PushdownExpected(t), pushdown, msg)
 
 					targetCommand := append(
 						bson.D{

diff --git a/integration/query_compat_test.go b/integration/query_compat_test.go
@@ -131,14 +131,14 @@ func testQueryCompatWithProviders(t *testing.T, providers shareddata.Providers,
 					resultPushdown := tc.resultPushdown
 
 					var msg string
-					if setup.FilterPushdownDisabled() {
+					if setup.PushdownDisabled() {
 						resultPushdown = noPushdown
 						msg = "Filter pushdown is disabled, but target resulted with pushdown"
 					}
 
 					doc := ConvertDocument(t, explainRes)
 					pushdown, _ := doc.Get("filterPushdown")
-					assert.Equal(t, resultPushdown.FilterPushdownExpected(t), pushdown, msg)
+					assert.Equal(t, resultPushdown.PushdownExpected(t), pushdown, msg)
 
 					targetCursor, targetErr := targetCollection.Find(ctx, filter, opts)
 					compatCursor, compatErr := compatCollection.Find(ctx, filter, opts)
@@ -253,18 +253,16 @@ func TestQueryCappedCollectionCompat(t *testing.T) {
 		},
 		"Sort": {
 			sort:         bson.D{{"_id", int32(-1)}},
-			sortPushdown: pgPushdown,
+			sortPushdown: noPushdown,
 		},
 		"FilterSort": {
 			filter:       bson.D{{"v", int32(42)}},
 			sort:         bson.D{{"_id", int32(-1)}},
-			sortPushdown: pgPushdown,
+			sortPushdown: noPushdown,
 		},
 		"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.
-			sortPushdown: allPushdown,
+			sort:         bson.D{{"v", 1}, {"_id", int32(-1)}},
+			sortPushdown: noPushdown,
 		},
 	} {
 		name, tc := name, tc
@@ -289,7 +287,7 @@ func TestQueryCappedCollectionCompat(t *testing.T) {
 
 			doc := ConvertDocument(t, explainRes)
 			sortPushdown, _ := doc.Get("sortPushdown")
-			assert.Equal(t, tc.sortPushdown.SortPushdownExpected(t), sortPushdown)
+			assert.Equal(t, tc.sortPushdown.PushdownExpected(t), sortPushdown)
 
 			findOpts := options.Find()
 			if tc.sort != nil {

diff --git a/integration/query_test.go b/integration/query_test.go
@@ -774,17 +774,17 @@ func TestQueryCommandLimitPushDown(t *testing.T) {
 
 				doc := ConvertDocument(t, res)
 				limitPushdown, _ := doc.Get("limitPushdown")
-				assert.Equal(t, tc.limitPushdown.SortPushdownExpected(t), limitPushdown)
+				assert.Equal(t, tc.limitPushdown.PushdownExpected(t), limitPushdown)
 
 				var msg string
 
-				if setup.FilterPushdownDisabled() {
+				if setup.PushdownDisabled() {
 					tc.filterPushdown = noPushdown
 					msg = "Filter pushdown is disabled, but target resulted with pushdown"
 				}
 
 				filterPushdown, _ := ConvertDocument(t, res).Get("filterPushdown")
-				assert.Equal(t, tc.filterPushdown.FilterPushdownExpected(t), filterPushdown, msg)
+				assert.Equal(t, tc.filterPushdown.PushdownExpected(t), filterPushdown, msg)
 			})
 
 			t.Run("Find", func(t *testing.T) {

diff --git a/integration/setup/helpers.go b/integration/setup/helpers.go
@@ -84,9 +84,9 @@ func SkipForMongoDB(tb testtb.TB, reason string) {
 	}
 }
 
-// FilterPushdownDisabled returns true if FerretDB filter pushdown is disabled.
-func FilterPushdownDisabled() bool {
-	return *disableFilterPushdownF
+// PushdownDisabled returns true if FerretDB pushdown is disabled.
+func PushdownDisabled() bool {
+	return *disablePushdownF
 }
 
 // Dir returns the absolute directory of this package.

diff --git a/integration/setup/listener.go b/integration/setup/listener.go
@@ -179,9 +179,9 @@ func setupListener(tb testtb.TB, ctx context.Context, logger *zap.Logger) string
 		HANAURL:       *hanaURLF,
 
 		TestOpts: registry.TestOpts{
-			DisableFilterPushdown: *disableFilterPushdownF,
-			EnableOplog:           true,
-			EnableNewAuth:         false,
+			DisablePushdown: *disablePushdownF,
+			EnableOplog:     true,
+			EnableNewAuth:   false,
 		},
 	}
 	h, closeBackend, err := registry.NewHandler(handler, handlerOpts)

diff --git a/integration/setup/setup.go b/integration/setup/setup.go
@@ -59,7 +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")
+	disablePushdownF = flag.Bool("disable-pushdown", false, "disable pushdown")
 )
 
 // Other globals.

diff --git a/internal/backends/collection.go b/internal/backends/collection.go
@@ -76,9 +76,10 @@ func CollectionContract(c Collection) Collection {
 
 // QueryParams represents the parameters of Collection.Query method.
 type QueryParams struct {
-	Filter        *types.Document
-	Sort          *types.Document
-	Limit         int64
+	Filter *types.Document
+	Sort   *types.Document
+	Limit  int64
+
 	OnlyRecordIDs bool
 	Comment       string
 }
@@ -96,7 +97,14 @@ type QueryResult struct {
 // It also can be used to close the returned iterator and free underlying resources,
 // but doing so is not necessary - the handler will do that anyway.
 //
-// Passed sort document should be already validated. If sort document is invalid, function panics.
+// Filter may be ignored, or safely applied partially or entirely.
+// Extra documents will be filtered out by the handler.
+//
+// Sort should have one of the following forms: nil, {}, {"$natural": int64(1)} or {"$natural": int64(-1)}.
+// Other field names are not supported.
+// If non-empty, it should be applied.
+//
+// Limit, if non-zero, should be applied.
 func (cc *collectionContract) Query(ctx context.Context, params *QueryParams) (*QueryResult, error) {
 	defer observability.FuncCall(ctx)()
 
@@ -105,24 +113,11 @@ func (cc *collectionContract) Query(ctx context.Context, params *QueryParams) (*
 	}
 
 	if params.Sort.Len() != 0 {
-		iter := params.Sort.Iterator()
-		defer iter.Close()
+		must.BeTrue(params.Sort.Len() == 1)
+		sortValue := params.Sort.Map()["$natural"].(int64)
 
-		for {
-			_, v, err := iter.Next()
-			if err != nil {
-				if errors.Is(err, iterator.ErrIteratorDone) {
-					break
-				}
-
-				return nil, lazyerrors.Error(err)
-			}
-
-			sortValue := v.(int64)
-
-			if sortValue != -1 && sortValue != 1 {
-				panic("sort key ordering must be 1 (for ascending) or -1 (for descending)")
-			}
+		if sortValue != -1 && sortValue != 1 {
+			panic("sort value must be 1 (for ascending) or -1 (for descending)")
 		}
 	}