Fix listIndexes bug with double quoted columnName in the index target from indexMetadata #1772

Yuqi-Du · 2024-12-02T20:21:42Z

What this PR does:
Summary, we got something like this, columnName is doubleQuoted in index target.

The regex does not match it.
We need to manually remove the double quote and then create cqlIndentifier.

Reproduction:

Table

{"createTable": {
  "name": "dreams",
  "definition": {
    "columns": {
      "matchId": "text",
      "round": "tinyint",
      "mVector": { "type": "vector", "dimension": 3 },
      "score": "int",
      "when": "timestamp",
      "winner": "text",
      "fighters": { "type": "set", "valueType": "uuid" }
    },
    "primaryKey": { "partitionBy": [ "matchId" ], "partitionSort": { "round": 1 } }
  },
  "options": { "ifNotExists": true }
}}

Index

{"createVectorIndex": {
  "name": "m_vector_index",
  "definition": {
    "column": "mVector",
    "options": { "sourceModel": "ada002", "metric": "dot_product" }
  }
}}

(unknown-returning) List indexes

{
    "status": {
        "indexes": [
            {
                "name": "abc_AH_123_a_1121",
                "definition": {
                    "column": "UNKNOWN",
                    "apiSupport": {
                        "createIndex": false,
                        "filter": false,
                        "cqlDefinition": "CREATE CUSTOM INDEX \"abc_AH_123_a_1121\" ON filter.testindex (\"V22eector2\")\nUSING 'StorageAttachedIndex'\nWITH OPTIONS = {\n    'similarity_function' : 'DOT_PRODUCT',\n    'source_model' : 'ADA002'}"
                    }
                }
            }
       }
}

Which issue(s) this PR fixes:
Fixes #1770

Checklist

Changes manually tested
Automated Tests added/updated
Documentation added/updated
CLA Signed: DataStax CLA

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java

tatu-at-datastax

Ok this looks quite complicated despite small size of diffs.

I am not super happy with the way things are handled but it may be necessary to fix the issue so that's ok.

But what really would be needed would be a test to show that fix works, if at all possible. And will guard against regression (avoid bug from reappearing due to changes)

…oted-problem

…oted-problem # Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/ApiIndexType.java # src/test/java/io/stargate/sgv2/jsonapi/api/v1/tables/CreateTableIndexIntegrationTest.java

amorton · 2025-01-13T20:15:26Z

I would not have merged this, it is a very good candidate to write a unit test , so I have re-opened the ticket #1770

amorton

-1 on this merging, it makes some assumptions about things in the CQL target value that are risks, removes some validation checks , and adds a new enum rather than use the existing ApiIndexFunction

amorton · 2025-01-13T20:17:04Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java

   */
-  private static Pattern INDEX_TARGET_PATTERN = Pattern.compile("^(\\w+)?(?:\\((\\w+)\\))?$");
+  private static final Pattern TARGET_REGEX =


This code is copied from the CQL class, we should add the name of the CQL class

amorton · 2025-01-13T20:20:41Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java

-      functionName = matcher.group(1);
+    } else {
+      columnName = target;
+      cqlIndexType = CqlIndexType.VALUES;


I am not sure about this, why is the default Values ?
What about what this is an index on a text field ?

amorton · 2025-01-13T20:24:33Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java

+      columnName = TWO_QUOTES.matcher(columnName).replaceAll(QUOTE);
+    }
+
+    return new IndexTarget(CqlIdentifier.fromInternal(columnName), cqlIndexType);


Why did this stop using existing ApiIndexFunction ?

amorton · 2025-01-13T20:25:39Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java

+      switch (this) {
+        case KEYS:
+          return "keys";
+        case KEYS_AND_VALUES:


why is this called KEY_AND_VALUES when it is the entries ?

amorton · 2025-01-13T20:26:15Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java

+
+    public static CqlIndexType fromString(String s) {
+      if ("".equals(s)) return SIMPLE;
+      else if ("values".equals(s)) return VALUES;


this is a case sensitive match and duplicates the logic in toString

amorton · 2025-01-13T20:27:15Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java

+      else if ("entries".equals(s)) return KEYS_AND_VALUES;
+      else if ("full".equals(s)) return FULL;
+
+      throw new AssertionError("Unrecognized index target type " + s);


This should be IllegalArgument we have not used AssertionError anywhere in the code

amorton · 2025-01-13T20:29:52Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/ApiVectorIndex.java

@@ -268,13 +268,6 @@ protected ApiIndexDef create(
                .formatted(ApiIndexType.VECTOR, apiIndexType));
      }

-      // also, we  must not have an index function


Why is this check removed ?

amorton · 2025-01-13T20:32:42Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/ApiIndexType.java

-    if (indexTarget.indexFunction() == null && apiColumnDef.type().isPrimitive()) {
+    // If cqlIndexType is values, and the column is a scalar
+    // then it is a regular index on primitive types
+    if (indexTarget.cqlIndexType() == CQLSAIIndex.CqlIndexType.VALUES


this feels risky - we are inferring the VALUES function when it is not defined in the CQL for the index and then using that inferred value. The previous code did not infer a function when there was none defined.

amorton · 2025-01-13T20:33:42Z

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/ApiRegularIndex.java

@@ -186,13 +186,6 @@ protected ApiIndexDef create(
                .formatted(ApiIndexType.REGULAR, apiIndexType));
      }

-      // also, we should not have an index function


why is this check removed, there is no function for a regular index

init

cb6eb60

Yuqi-Du requested a review from a team as a code owner December 2, 2024 20:21

tatu-at-datastax reviewed Dec 2, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed Dec 2, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed Dec 2, 2024

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/CQLSAIIndex.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed Dec 2, 2024

View reviewed changes

tatu-at-datastax and others added 5 commits December 4, 2024 15:58

Merge branch 'main' into yuqi/fix-listIndex-quoted-problem

79df8d0

Merge remote-tracking branch 'origin/main' into yuqi/fix-listIndex-qu…

8f87146

…oted-problem

fix comments and add IT

881e2bc

format

6aaae7c

Merge branch 'main' into yuqi/fix-listIndex-quoted-problem

1cc376b

tatu-at-datastax approved these changes Dec 6, 2024

View reviewed changes

Yuqi-Du and others added 5 commits December 9, 2024 14:20

Merge branch 'main' into yuqi/fix-listIndex-quoted-problem

e04b2df

Merge branch 'main' into yuqi/fix-listIndex-quoted-problem

28f6037

Merge remote-tracking branch 'origin/main' into yuqi/fix-listIndex-qu…

7146f25

…oted-problem # Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/schema/tables/ApiIndexType.java # src/test/java/io/stargate/sgv2/jsonapi/api/v1/tables/CreateTableIndexIntegrationTest.java

align parsing logic of indexTarget to what cassandra does

71e53d2

fix it

63c538c

Yuqi-Du merged commit f033b0e into main Jan 13, 2025
3 checks passed

Yuqi-Du deleted the yuqi/fix-listIndex-quoted-problem branch January 13, 2025 19:59

Yuqi-Du mentioned this pull request Jan 13, 2025

Revert "Fix listIndexes bug with double quoted columnName in the index target from indexMetadata" #1823

Merged

Yuqi-Du restored the yuqi/fix-listIndex-quoted-problem branch January 13, 2025 20:36

amorton reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix listIndexes bug with double quoted columnName in the index target from indexMetadata #1772

Fix listIndexes bug with double quoted columnName in the index target from indexMetadata #1772

Yuqi-Du commented Dec 2, 2024

tatu-at-datastax left a comment

amorton commented Jan 13, 2025

amorton left a comment

amorton Jan 13, 2025

amorton Jan 13, 2025

amorton Jan 13, 2025

amorton Jan 13, 2025

amorton Jan 13, 2025

amorton Jan 13, 2025

amorton Jan 13, 2025

amorton Jan 13, 2025

amorton Jan 13, 2025

Fix listIndexes bug with double quoted columnName in the index target from indexMetadata #1772

Fix listIndexes bug with double quoted columnName in the index target from indexMetadata #1772

Conversation

Yuqi-Du commented Dec 2, 2024

tatu-at-datastax left a comment

Choose a reason for hiding this comment

amorton commented Jan 13, 2025

amorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment