Merge pull request #43 from hasura/m-bilal/fix-subfield-extraction

make `internal.ExtractTypes` return subFieldMap

connector/fields.go

@@ -49,7 +49,7 @@ func handleFieldTypeAggregateMetricDouble(fieldMap map[string]interface{}) {
 // This compound scalar type supports a superset of comparison and aggregation operations of all its subtypes and the actualType
 // This compund scalar type is added to the scalarTypeMap before being returned
 func GetFieldType(fieldMap map[string]interface{}, state *types.State, indexName string, fieldName string) string {
-	fieldTypes := internal.ExtractTypes(fieldMap)
+	fieldTypes, _, _ := internal.ExtractTypes(fieldMap)
 	actualFieldType := fieldTypes[0] // actualFieldType is the type type of the field that the db has. It is the main type, not the subtype
 
 	if len(fieldTypes) > 1 {

connector/fields_test.go

@@ -26,6 +26,9 @@ var tests = []test{
 	{
 		name: "books",
 	},
+	{
+		name: "books_2",
+	},
 }
 
 func TestSchema(t *testing.T) {

connector/query_test.go

@@ -85,11 +85,15 @@ var tests = []test{
 		group: "payments",
 		name:  "simple_terms_clause",
 	},
+	{
+		group: "customers",
+		name:  "sort_by_subtype",
+	},
 }
 
 func TestPrepareElasticsearchQuery(t *testing.T) {
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+		t.Run(tt.group + "." + tt.name, func(t *testing.T) {
 			ctx := context.Background()
 			ctx = context.WithValue(ctx, "postProcessor", &types.PostProcessor{})
 			initTest(t, &tt)

connector/sort.go

@@ -39,18 +39,16 @@ func prepareSortElement(element *schema.OrderByElement, state *types.State, coll
 			})
 		}
 
-		fieldType, fieldSubTypes, fieldDataEnabled, err := state.Configuration.GetFieldProperties(collection, fieldPath)
+		fieldType, subFieldMap, fieldDataEnabled, err := state.Configuration.GetFieldProperties(collection, fieldPath)
 		if err != nil {
 			return nil, schema.InternalServerError("failed to get field types", map[string]any{"error": err.Error()})
 		}
 
 		if !internal.IsSortSupported(fieldType, fieldDataEnabled) {
-			// we iterate over the fieldSubTypes in reverse because the subtypes are sorted by priority.
-			// We want to use the highest priority subType that is supported for sorting.
-			for i := len(fieldSubTypes) - 1; i >= 0; i-- {
-				subType := fieldSubTypes[i]
-				if internal.IsSortSupported(subType, fieldDataEnabled) {
-					validField = fmt.Sprintf("%s.%s", validField, subType)
+			// since the type does not support sorting, we iterate over the subfields to find a subfield that does
+			for subFieldType, subField := range subFieldMap {
+				if internal.IsSortSupported(subFieldType, fieldDataEnabled) {
+					validField = fmt.Sprintf("%s.%s", validField, subField)
 					break
 				}
 			}

internal/fields.go

@@ -1,18 +1,38 @@
 package internal
 
 // Given a fieldMap, this function extracts the type and all subtypes (if present)
-func ExtractTypes(fieldMap map[string]interface{}) (fieldAndSubfields []string) {
+//
+// **RETURNS**
+// 1. legacyFieldAndSubfields: a slice of strings containing the field type as the first element and all subfields in the following elements. Called legacy because it supports older code that expects this format. Please refrain from using this in newer functions.
+// 2. fieldType: the type of the field
+// 3. subFieldsMap: a map of types to their subfields
+func ExtractTypes(fieldMap map[string]interface{}) (legacyFieldAndSubfields []string, fieldType string, subFieldsMap map[string]string) {
+	subFieldsMap = make(map[string]string) // subFieldsMap is a map of types and their subFields
+	fieldType, _ = FieldTypeIsScalar(fieldMap)
+
 	if subFields, ok := HasSubfields(fieldMap); ok {
-		for _, subFieldData := range subFields {
-			fieldAndSubfields = append(fieldAndSubfields, ExtractTypes(subFieldData.(map[string]interface{}))...)
+		for subField, subFieldData := range subFields {
+			subFieldType, ok := subFieldData.(map[string]interface{})["type"].(string)
+			if !ok {
+				continue
+			}
+			if subFieldType == fieldType {
+				// since the subfield type is the same as the field type, we will consider the main field type
+				continue
+			}
+			if _, ok := subFieldsMap[subFieldType]; ok {
+				// subFieldType already exists, skip
+				continue
+			}
+			legacyFieldAndSubfields = append(legacyFieldAndSubfields, subFieldType)
+			subFieldsMap[subFieldType] = subField
 		}
 	}
 
-	SortTypesByPriority(fieldAndSubfields)
+	SortTypesByPriority(legacyFieldAndSubfields)
 
-	fieldType, _ := FieldTypeIsScalar(fieldMap)
-	fieldAndSubfields = append([]string{fieldType}, fieldAndSubfields...)
-	return fieldAndSubfields
+	legacyFieldAndSubfields = append([]string{fieldType}, legacyFieldAndSubfields...)
+	return legacyFieldAndSubfields, fieldType, subFieldsMap
 }
 
 func HasSubfields(fieldMap map[string]interface{}) (subFields map[string]interface{}, ok bool) {
@@ -24,7 +44,7 @@ func HasSubfields(fieldMap map[string]interface{}) (subFields map[string]interfa
  * This function checks if a field is a scalar field
  * Scalar field here refers to a field that does not have any nested fields/properties
  */
- func FieldTypeIsScalar(fieldMap map[string]interface{}) (fieldType string, isFieldScalar bool) {
+func FieldTypeIsScalar(fieldMap map[string]interface{}) (fieldType string, isFieldScalar bool) {
 	fieldType, ok := fieldMap["type"].(string)
 	return fieldType, ok && fieldType != "nested" && fieldType != "object" && fieldType != "flattened"
 }
@@ -65,4 +85,4 @@ func IsFieldDtaEnabled(fieldMap map[string]interface{}) bool {
 		fieldDataEnalbed = fieldData
 	}
 	return fieldDataEnalbed
-}
+}

internal/fields_test.go

@@ -0,0 +1,132 @@
+package internal
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var fieldsAndSubfieldsTests = []struct {
+	name                        string
+	fieldMapStr                 string
+	wantLegacyFieldAndSubfields []string
+	wantType                    string
+	wantSubFieldsMap            map[string]string
+}{
+	{
+		name: "nested_field",
+		fieldMapStr: `{
+        	"type": "text",
+        	"fields": {
+          		"raw": { 
+            		"type":  "keyword"
+          		}
+        	}
+      	}`,
+		wantLegacyFieldAndSubfields: []string{"text", "keyword"},
+		wantType:                    "text",
+		wantSubFieldsMap:            map[string]string{"keyword": "raw"},
+	},
+	{
+		name: "nested_fields",
+		fieldMapStr: `{
+        	"type": "text",
+        	"fields": {
+          		"raw": {
+            		"type":  "keyword"
+          		},
+				"raw_int": {
+            		"type":  "integer"
+          		},
+				"raw_double": { 
+            		"type":  "double"
+          		},
+				"simple_boolean": {
+            		"type":  "boolean"
+          		},
+				"ip": {
+            		"type":  "ip"
+          		},
+				"version": {
+            		"type":  "version"
+          		}
+        	}
+      	}`,
+		wantLegacyFieldAndSubfields: []string{
+			"text",
+			"boolean",
+			"integer",
+			"double",
+			"ip",
+			"keyword",
+			"version",
+		},
+		wantType: "text",
+		wantSubFieldsMap: map[string]string{
+			"keyword": "raw",
+			"integer": "raw_int",
+			"double":  "raw_double",
+			"boolean": "simple_boolean",
+			"ip":      "ip",
+			"version": "version",
+		},
+	},
+	{
+		name: "no_nested_field",
+		fieldMapStr: `{
+        	"type": "keyword"
+      	}`,
+		wantLegacyFieldAndSubfields: []string{"keyword"},
+		wantType:                    "keyword",
+		wantSubFieldsMap:            map[string]string{},
+	},
+	{
+		name: "nested_field_type_same_as_field_type",
+		fieldMapStr: `{ 
+        	"type": "text",
+        	"fields": {
+          		"english": { 
+            		"type": "text",
+            		"analyzer": "english"
+          		}
+        	}
+      	}`,
+		wantLegacyFieldAndSubfields: []string{"text"},
+		wantType: "text",
+		wantSubFieldsMap: map[string]string{},
+	},
+	{
+		name: "duplicate_nested_field",
+		fieldMapStr: `{
+        	"type": "text",
+        	"fields": {
+          		"raw": { 
+            		"type":  "keyword"
+          		},
+				"raw": { 
+            		"type":  "keyword"
+          		}
+        	}
+      	}`,
+		wantLegacyFieldAndSubfields: []string{"text", "keyword"},
+		wantType:                    "text",
+		wantSubFieldsMap:            map[string]string{"keyword": "raw"},
+	},
+}
+
+func TestExtractTypes(t *testing.T) {
+	for _, tt := range fieldsAndSubfieldsTests {
+		t.Run(tt.name, func(t *testing.T) {
+			var fieldMap map[string]interface{}
+			err := json.Unmarshal([]byte(tt.fieldMapStr), &fieldMap)
+			assert.NoError(t, err, "Error unmarshalling JSON")
+
+			fieldAndSubfields, fieldType, subFieldsMap := ExtractTypes(fieldMap)
+
+			assert.Equal(t, tt.wantLegacyFieldAndSubfields, fieldAndSubfields)
+			assert.Equal(t, tt.wantType, fieldType)
+			assert.Equal(t, tt.wantSubFieldsMap, subFieldsMap)
+		})
+	}
+}

testdata/unit_tests/fields_tests/books_2/configuration.json

@@ -0,0 +1,38 @@
+{
+  "indices": {
+    "my_book_index": {
+      "mappings": {
+        "properties": {
+          "author": {
+            "type": "keyword"
+          },
+          "description": {
+            "type": "text"
+          },
+          "genre": {
+            "type": "keyword"
+          },
+          "pages": {
+            "type": "integer"
+          },
+          "published_date": {
+            "format": "yyyy-MM-dd",
+            "type": "date"
+          },
+          "rating": {
+            "type": "float"
+          },
+          "title": {
+            "fields": {
+              "keywordSubField": {
+                "type": "keyword"
+              }
+            },
+            "type": "text"
+          }
+        }
+      }
+    }
+  },
+  "queries": {}
+}