New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CNDB-12503 remove column name restrictions in SAI #1539

Open

k-rus wants to merge 25 commits into main from rf-cndb-12503-sai-syntax

+248 −40

k-rus commented Jan 30, 2025 •

edited

Loading

What is the issue

Trying to create an SAI or other index on a column, which name is either long or has
non-alphanumeric characters, fails, while the table was successfully
created with such name. This happens if an index name is not provided.
The reason for this limitation is that the column name is used in
the default index name, which is used in the file names.

What does this PR fix and why was it fixed

Fixes https://github.com/riptano/cndb/issues/12503, fixes https://github.com/riptano/cndb/issues/12609

This PR removes the check for column names during an index creation. Thus it allows to create indexes on existing columns with any names including qualified names and long names.

The check was preventing qualified column names with characters, which cannot be used in file names. This was never an issue as the index name construction from table and column names already removes any non-alphanumeric or non-underscore characters.
The check was preventing long column names. This limitation is removed by

Adding calculation of maximum amount characters, which can be used in index name, so it will allow to construct file names without exceeding the limit of 255 chars (255 is added as a constant).
Then the index name is truncated.

The uniqueness of index names is already fixed by existing implementation, which adds counter prefix when needed.

This change to generated index name does not affect existing indexes as the existing name from IndexMetadata is always serialized to JSON, thus avoiding to re-generating the name.

This fix required to fix CQL tester, since it was assuming that the restriction is in place. So column name regex pattern matcher is fixed in createIndex to allow non-restricted column names. There might still be possible corner cases on some column names used for generating index names, which will not work with createIndex. In such case execute should be used.

It also includes several fixes in affected files:

Improve the name and usage of constants.
Fix warnings with minor improvements in affected files.

Checklist before you submit for review

Make sure there is a PR in the CNDB project updating the Converged Cassandra version
Use NoSpamLogger for log lines that may appear frequently in the logs
Verify test results on Butler
Test coverage for new/modified code is > 80%
Proper code formatting
Proper title for each commit staring with the project-issue number, like CNDB-1234
Each commit has a meaningful description
Each commit is not very long and contains related changes
Renames, moves and reformatting are in distinct commits

k-rus force-pushed the rf-cndb-12503-sai-syntax branch from 5f71894 to 569922d Compare

January 30, 2025 10:20

k-rus requested a review from a team

January 30, 2025 11:16

k-rus force-pushed the rf-cndb-12503-sai-syntax branch from 569922d to 12441d7 Compare

January 31, 2025 11:03

pkolaczk requested changes

View reviewed changes

src/java/org/apache/cassandra/cql3/statements/schema/AlterSchemaStatement.java Outdated Show resolved Hide resolved

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated

+                  private static int calculateAllowedLength(int keyspaceNameLength, int tableNameLength)
+                  {
+                      // Prefixes and suffixes from IndexContext.getFullName
+                      int addedLength1 = keyspaceNameLength + tableNameLength + 2 + 4;

pkolaczk Jan 31, 2025

Magic numbers - can you make them into named constants?

Author

k-rus Jan 31, 2025

I also prefer to avoid magic numbers. In this case I started with constants, but it became unreadable due to many constants used only once and capitalized names does not look readable. I am open for suggestion how to make maintainable and correct code without investing too much time.

Author

k-rus Feb 3, 2025

I exploded the code with named variables for values and as result putting into different functions as recommended by IntelliJ.

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated

Comment on lines 117 to 120

+                      // Prefixes and suffixes constructed by Version.stargazerFileNameFormat
+                      int addedLength2 = SAI_DESCRIPTOR.length() + 4 + IndexComponentType.KD_TREE_POSTING_LISTS.representation.length() + 3;
+                      // Prefixes from Descriptor constructor
+                      addedLength2 += 2 + 2 + SSTableFormat.Type.BTI.name().length() + 16;

pkolaczk Jan 31, 2025

Same here: those constant numbers scream they need names

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated Show resolved Hide resolved

test/unit/org/apache/cassandra/index/sai/cql/QuotedIdentifierColumnTest.java Outdated Show resolved Hide resolved

k-rus requested review from pkolaczk and a team

February 3, 2025 11:42

k-rus changed the title ~~CNDB-12503 remove column restrictions in SAI~~ CNDB-12503 remove column name restrictions in SAI

k-rus force-pushed the rf-cndb-12503-sai-syntax branch 2 times, most recently from cc08613 to c6cd552 Compare

February 3, 2025 17:47

Author

k-rus commented Feb 4, 2025

Fixed usage of renames in CNDB: https://github.com/riptano/cndb/pull/12715

adelapena reviewed

View reviewed changes

test/unit/org/apache/cassandra/index/sai/cql/NativeIndexDDLTest.java Outdated Show resolved Hide resolved

k-rus requested a review from adelapena

February 7, 2025 09:20

adelapena reviewed

View reviewed changes

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated Show resolved Hide resolved

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated Show resolved Hide resolved

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated Show resolved Hide resolved

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated

+                   */
+                  private static int calculateGeneratedIndexNameLength(int keyspaceNameLength, int tableNameLength)
+                  {
+                      int uniquenessSuffixLength = 3;

adelapena Feb 10, 2025

I understand this sets a maximum of 100 indexes with a long name per keyspace. So for example a keyspace with a very long name could have 20 tables with 5 indexes each. It looks like a reasonable limit, albeit I wonder if for peace of mind we should use a max suffix length of 4 chars.

In any case, I would add a comment here about how this limits the number of indexes with a long name, and perhaps put that number in a constant.

Author

k-rus Feb 10, 2025

Just to be fully correct it will be limited to 100 only in the case of long keyspace and table names. An index name is table + column names and it will be trimmed to allow later addition of keyspace and table names.
Currently the limit of 3 chars suffix is soft, i.e., if this condition satisfied, then later it can be more than 3 chars be added as it's not controlled there. It will not fail until the number of chars is big enough, since the name length constant of 222 chars is arbitrary to my understanding and is below the file name limit by OSs.
I avoid this corner case with uniqueness suffix, I think to control the result index name to fit the allowed size as it's now done only for the given index name.

Author

k-rus Mar 5, 2025

I increased to 4 chars, i.e., 999 repetition of the same index name. It should be safe enough. There is still a bug of not validating length of index names, https://github.com/riptano/cndb/issues/13198

src/java/org/apache/cassandra/schema/IndexMetadata.java Outdated Show resolved Hide resolved

adelapena reviewed

View reviewed changes

test/unit/org/apache/cassandra/index/sai/cql/NativeIndexDDLTest.java Outdated Show resolved Hide resolved

k-rus added 16 commits

March 3, 2025 16:55


          Improve related files with minor fixes

dc169d1

Several fixes:
- Improve the name of the utility function to check database object
names to provide more semantics.
- Remove duplicates of utility functions and constants.
- Fix warnings with minor improvements in affected files.


          CNDB-12503 remove column restrictions in SAI

c9ef22c

Also fix CNDB-12609

Trying to create an SAI on a column, which name is either long or has
non-alphanumeric characters, fails, while the table was successfully
created with such name. This happens if an index name is not provided.
The reason for this limitation is that the column name is used in
the default index name, which is used in the file names.

This commit fixes this limitation by removing characters other than
alphabetic, numeric or underscore when creating an index name and
trimming the index name to be acceptable for a file name.


          Restore UNQUOTED_IDENTIFIER in accordance to documentation

e5b04d2


          CNDB-12503 Add long name tests and give constant names

97a4093

Adds tests with long keyspace and table names.
Improves readability of calculating additional future length by
putting values into variables.


          CNDB-12503 clean up minor

dff473e


          CNDB-12503 revert test to create index on any created column

32b71ed


          CNDB-12503 move tests into existing create index test file

9b62633


          CNDB-12503 improve prefix name check

2467d4c


          CNDB-12503 fix minor warnings by IntelliJ in affected file

0a2e41f


          CNDB-12503 fix tester for quoted column based index names

87cecdf

CQLTester.createIndex assumed non-qualified column names. Thus it was
necessary to adjust it, since this PR removes the restriction.


          CNDB-12609 validate length of constructing filename from given index …

f5641ed

…name


          CNDB-12503 assert except messages and fix the error messages

a9b77bd


          CNDB-12503 remove generated index suffix from allowed index name length

7a967b3

Allowed length of index name is used to check given index in addition to
truncate generated index name. The generated name needs also to take in
account the generated suffix. Thus, splitting in tow functions.


          CNDB-12503 fix back assumption that column name is provided in low case

27d3257

When an index name is generated from a column name, it keeps the same
capitalization for qualified column name, while SQL tester and existing
tests assume that any column name is switched to lower case only.


          CNDB-12503 keep the case for generated index from quoted column name

8a2f0d4


          CNDB-12503 update test error message to actual

fa4e3d8

k-rus added 4 commits

March 3, 2025 16:58


          CNDB-12503 improve name and comments


          CNDB-12503 use ConfigurationException as more appropriate

8985b2f


          CNDB-12503 change exception and improve names and comments

2c90249


          CNDB-12503 explain the length of uniqueness suffix

fc5f50e

k-rus force-pushed the rf-cndb-12503-sai-syntax branch from 2f4f862 to fc5f50e Compare

March 3, 2025 16:06

k-rus added 4 commits

March 4, 2025 21:53


          Correct index name length calculation

c6afff0

After checking through code and test it was found that only one option
is used for file name with index name in it. So it's corrected and the
logic is moved into SAI.

Removed validation of index name length to not break existing indexes.


          Fix warning in affected file

4f11c20


          CNDB-12503 add tests for different indexes and index name

f909afc


          Reverse formatting

fa313cb

k-rus force-pushed the rf-cndb-12503-sai-syntax branch from ad74ff0 to fa313cb Compare

March 5, 2025 13:06


          Clean up names and comments

88a8dcb

k-rus requested a review from adelapena

March 5, 2025 13:29

sonarqubecloud bot commented Mar 5, 2025

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

cassci-bot commented Mar 5, 2025

✔️ Build ds-cassandra-pr-gate/PR-1539 approved by Butler

Approved by Butler
See build details here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet