Temporarily remove pushdown for dot notation (#2068)

Closes #2055.
FerretDB · Mar 1, 2023 · 970cdf2 · 970cdf2
1 parent fa396d4
commit 970cdf2
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 87 deletions.
diff --git a/integration/query_array_compat_test.go b/integration/query_array_compat_test.go
@@ -84,14 +84,12 @@ func testQueryArrayCompatDotNotation() map[string]queryCompatTestCase {
 			resultType: emptyResult,
 		},
 		"Field": {
-			filter:         bson.D{{"v.array", int32(42)}},
-			skipForTigris:  "Tigris does not support language keyword 'array' as field name",
-			resultPushdown: true,
+			filter:        bson.D{{"v.array", int32(42)}},
+			skipForTigris: "Tigris does not support language keyword 'array' as field name",
 		},
 		"FieldPosition": {
-			filter:         bson.D{{"v.array.0", int32(42)}},
-			skipForTigris:  "Tigris does not support language keyword 'array' as field name",
-			resultPushdown: true,
+			filter:        bson.D{{"v.array.0", int32(42)}},
+			skipForTigris: "Tigris does not support language keyword 'array' as field name",
 		},
 		"FieldPositionQuery": {
 			filter:        bson.D{{"v.array.0", bson.D{{"$gte", int32(42)}}}},
@@ -102,44 +100,32 @@ func testQueryArrayCompatDotNotation() map[string]queryCompatTestCase {
 			resultType: emptyResult,
 		},
 		"DocumentDotNotationArrayDocument": {
-			filter:         bson.D{{"v.0.foo.0.bar", "hello"}},
-			skipForTigris:  "No suitable Tigris-compatible provider to test this data",
-			resultPushdown: true,
+			filter:        bson.D{{"v.0.foo.0.bar", "hello"}},
+			skipForTigris: "No suitable Tigris-compatible provider to test this data",
 		},
 		"DocumentDotNotationArrayDocumentNoIndex": {
 			filter: bson.D{{"v.foo.bar", "hello"}},
 			skip:   "https://github.com/FerretDB/FerretDB/issues/1828",
 		},
 		"FieldArrayIndex": {
-			filter:         bson.D{{"v.foo[0]", int32(42)}},
-			skipForTigris:  "Tigris does not support characters as field name",
-			resultPushdown: true,
+			filter:        bson.D{{"v.foo[0]", int32(42)}},
+			skipForTigris: "Tigris does not support characters as field name",
 		},
 		"FieldArrayAsterix": {
-			filter:         bson.D{{"v.foo[*]", int32(42)}},
-			skipForTigris:  "Tigris does not support characters as field name",
-			resultPushdown: true,
+			filter:        bson.D{{"v.foo[*]", int32(42)}},
+			skipForTigris: "Tigris does not support characters as field name",
 		},
 		"FieldAsterix": {
-			filter:         bson.D{{"v.*", int32(42)}},
-			skipForTigris:  "Tigris does not support characters as field name",
-			resultPushdown: true,
+			filter:        bson.D{{"v.*", int32(42)}},
+			skipForTigris: "Tigris does not support characters as field name",
 		},
 		"FieldAt": {
-			filter:         bson.D{{"v.@", int32(42)}},
-			skipForTigris:  "Tigris does not support characters as field name",
-			resultPushdown: true,
+			filter:        bson.D{{"v.@", int32(42)}},
+			skipForTigris: "Tigris does not support characters as field name",
 		},
 		"FieldComma": {
-			filter:         bson.D{{"v.f,oo", int32(42)}},
-			skipForTigris:  "Tigris does not support characters as field name",
-			resultPushdown: true,
-		},
-		"FieldDollarSign": {
-			filter:         bson.D{{"v.$", int32(42)}},
-			skipForTigris:  "Tigris does not support characters as field name",
-			resultPushdown: true,
-			resultType:     emptyResult,
+			filter:        bson.D{{"v.f,oo", int32(42)}},
+			skipForTigris: "Tigris does not support characters as field name",
 		},
 	}
 

diff --git a/integration/query_comparison_compat_test.go b/integration/query_comparison_compat_test.go
@@ -47,14 +47,12 @@ func testQueryComparisonCompatImplicit() map[string]queryCompatTestCase {
 			resultType: emptyResult,
 		},
 		"DocumentDotNotation": {
-			filter:         bson.D{{"v.foo", int32(42)}},
-			skipForTigris:  "No suitable Tigris-compatible provider to test this data",
-			resultPushdown: true,
+			filter:        bson.D{{"v.foo", int32(42)}},
+			skipForTigris: "No suitable Tigris-compatible provider to test this data",
 		},
 		"DocumentDotNotationNoSuchField": {
-			filter:         bson.D{{"no-such-field.some", 42}},
-			resultType:     emptyResult,
-			resultPushdown: true,
+			filter:     bson.D{{"no-such-field.some", 42}},
+			resultType: emptyResult,
 		},
 		"ArrayNoSuchField": {
 			filter:     bson.D{{"no-such-field", bson.A{42}}},

diff --git a/internal/handlers/pg/pgdb/query.go b/internal/handlers/pg/pgdb/query.go
@@ -251,11 +251,6 @@ func prepareWhereClause(sqlFilters *types.Document) (string, []any, error) {
 			return "", nil, lazyerrors.Error(err)
 		}
 
-		keyOperator := "->" // keyOperator is the operator that is used to access the field. (->/#>)
-
-		var key any = k   // key can be either a string '"v"' or PostgreSQL path '{v,foo}'
-		var prefix string // prefix is the first key in path, if the filter key is not a path - the prefix is empty
-
 		if k != "" {
 			// skip $comment
 			if k[0] == '$' {
@@ -267,31 +262,28 @@ func prepareWhereClause(sqlFilters *types.Document) (string, []any, error) {
 				return "", nil, lazyerrors.Error(err)
 			}
 
-			// If the key is in dot notation use path operator (#>)
+			// TODO dot notation https://github.com/FerretDB/FerretDB/issues/2069
 			if path.Len() > 1 {
-				keyOperator = "#>"
-				key = path.Slice()     // '{v,foo}'
-				prefix = path.Prefix() // 'v'
+				continue
 			}
 		}
 
 		// Handle _id with a simpler query, as it can't be an array
-		if k == "_id" || prefix == "_id" {
+		if k == "_id" {
 			switch v := v.(type) {
 			case *types.Document, *types.Array, types.Binary, bool, time.Time, types.NullType, types.Regex, types.Timestamp:
 				// type not supported for pushdown
 				// TODO $eq and $ne https://github.com/FerretDB/FerretDB/issues/1840
 				// TODO $gt and $lt https://github.com/FerretDB/FerretDB/issues/1875
 			case float64, string, types.ObjectID, int32, int64:
 				// Select if value under the key is equal to provided value.
-				sql := `((_jsonb%[1]s%[2]s)::jsonb = %[3]s)`
+				sql := `(_jsonb->%[1]s)::jsonb = %[2]s`
 
-				// operator is -> for non-array, and #> for array
 				// placeholder p.Next() returns SQL argument references such as $1, $2 to prevent SQL injections.
-				// placeholder $1 is used for field key or it's path,
+				// placeholder $1 is used for field key,
 				// placeholder $2 is used for field value v.
-				filters = append(filters, fmt.Sprintf(sql, keyOperator, p.Next(), p.Next()))
-				args = append(args, key, string(must.NotFail(pjson.MarshalSingleValue(v))))
+				filters = append(filters, fmt.Sprintf(sql, p.Next(), p.Next()))
+				args = append(args, k, string(must.NotFail(pjson.MarshalSingleValue(v))))
 
 			default:
 				panic(fmt.Sprintf("Unexpected type of value: %v", v))
@@ -311,14 +303,13 @@ func prepareWhereClause(sqlFilters *types.Document) (string, []any, error) {
 			// Select if value under the key is equal to provided value.
 			// If the value under the key is not equal to v,
 			// but the value under the key k is an array - select if it contains the value equal to v.
-			sql := `((_jsonb%[1]s%[2]s)::jsonb = %[3]s) OR (_jsonb%[1]s%[2]s)::jsonb @> %[3]s`
+			sql := `(_jsonb->%[1]s)::jsonb = %[2]s OR (_jsonb->%[1]s)::jsonb @> %[2]s`
 
-			// operator is -> for non-array, and #> for array
 			// placeholder p.Next() returns SQL argument references such as $1, $2 to prevent SQL injections.
-			// placeholder $1 is used for field key or it's path,
+			// placeholder $1 is used for field key,
 			// placeholder $2 is used for field value v.
-			filters = append(filters, fmt.Sprintf(sql, keyOperator, p.Next(), p.Next()))
-			args = append(args, key, string(must.NotFail(pjson.MarshalSingleValue(v))))
+			filters = append(filters, fmt.Sprintf(sql, p.Next(), p.Next()))
+			args = append(args, k, string(must.NotFail(pjson.MarshalSingleValue(v))))
 
 		default:
 			panic(fmt.Sprintf("Unexpected type of value: %v", v))

diff --git a/internal/handlers/pg/pgdb/query_test.go b/internal/handlers/pg/pgdb/query_test.go
@@ -289,7 +289,7 @@ func TestPrepareWhereClause(t *testing.T) {
 	objectID := types.ObjectID{0x62, 0x56, 0xc5, 0xba, 0x0b, 0xad, 0xc0, 0xff, 0xee, 0xff, 0xff, 0xff}
 
 	// WHERE clauses occurring frequently in tests
-	whereEq := " WHERE ((_jsonb->$1)::jsonb = $2)"
+	whereEq := " WHERE (_jsonb->$1)::jsonb = $2"
 	whereEqOrContain := whereEq + " OR (_jsonb->$1)::jsonb @> $2"
 
 	for name, tc := range map[string]struct {
@@ -339,16 +339,13 @@ func TestPrepareWhereClause(t *testing.T) {
 			expected: whereEq,
 		},
 		"IDDotNotation": {
-			filter:   must.NotFail(types.NewDocument("_id.doc", "foo")),
-			expected: " WHERE ((_jsonb#>$1)::jsonb = $2)",
+			filter: must.NotFail(types.NewDocument("_id.doc", "foo")),
 		},
 		"DotNotation": {
-			filter:   must.NotFail(types.NewDocument("v.doc", "foo")),
-			expected: " WHERE ((_jsonb#>$1)::jsonb = $2) OR (_jsonb#>$1)::jsonb @> $2",
+			filter: must.NotFail(types.NewDocument("v.doc", "foo")),
 		},
 		"DotNotationArrayIndex": {
-			filter:   must.NotFail(types.NewDocument("v.arr.0", "foo")),
-			expected: " WHERE ((_jsonb#>$1)::jsonb = $2) OR (_jsonb#>$1)::jsonb @> $2",
+			filter: must.NotFail(types.NewDocument("v.arr.0", "foo")),
 		},
 	} {
 		name, tc := name, tc

diff --git a/internal/handlers/tigris/tigrisdb/query.go b/internal/handlers/tigris/tigrisdb/query.go
@@ -18,7 +18,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"strconv"
 	"time"
 
 	"github.com/tigrisdata/tigris-client-go/driver"
@@ -93,8 +92,6 @@ func BuildFilter(filter *types.Document) (string, error) {
 	res := map[string]any{}
 
 	for k, v := range filter.Map() {
-		key := k // key can be either a single key string '"v"' or Tigris dot notation '"v.foo"'
-
 		// TODO https://github.com/FerretDB/FerretDB/issues/1940
 		if v == "" {
 			continue
@@ -106,30 +103,17 @@ func BuildFilter(filter *types.Document) (string, error) {
 				continue
 			}
 
-			// If the key is in dot notation translate it to a tigris dot notation
 			var path types.Path
 			var err error
 
 			if path, err = types.NewPathFromString(k); err != nil {
 				return "", lazyerrors.Error(err)
 			}
 
+			// TODO dot notation https://github.com/FerretDB/FerretDB/issues/2069
+			// TODO https://github.com/FerretDB/FerretDB/issues/1914
 			if path.Len() > 1 {
-				indexSearch := false
-
-				// TODO https://github.com/FerretDB/FerretDB/issues/1914
-				for _, k := range path.Slice() {
-					if _, err := strconv.Atoi(k); err == nil {
-						indexSearch = true
-						break
-					}
-				}
-
-				if indexSearch {
-					continue
-				}
-
-				key = path.String() // '"v.foo"'
+				continue
 			}
 		}
 
@@ -143,7 +127,7 @@ func BuildFilter(filter *types.Document) (string, error) {
 				return "", lazyerrors.Error(err)
 			}
 
-			res[key] = json.RawMessage(rawValue)
+			res[k] = json.RawMessage(rawValue)
 		default:
 			panic(fmt.Sprintf("Unexpected type of field %s: %T", k, v))
 		}

diff --git a/internal/handlers/tigris/tigrisdb/query_test.go b/internal/handlers/tigris/tigrisdb/query_test.go
@@ -275,12 +275,10 @@ func TestBuildFilter(t *testing.T) {
 			expected: `{"_id":"foo"}`,
 		},
 		"IDDotNotation": {
-			filter:   must.NotFail(types.NewDocument("_id.doc", "foo")),
-			expected: `{"_id.doc":"foo"}`,
+			filter: must.NotFail(types.NewDocument("_id.doc", "foo")),
 		},
 		"DotNotation": {
-			filter:   must.NotFail(types.NewDocument("v.doc", "foo")),
-			expected: `{"v.doc":"foo"}`,
+			filter: must.NotFail(types.NewDocument("v.doc", "foo")),
 		},
 		"DotNotationArrayIndex": {
 			filter: must.NotFail(types.NewDocument("v.arr.0", "foo")),