Create index command implementation #1526
Conversation
maheshrajamani
changed the title
maheshrajamani
changed the title
src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/override/ExtendedVectorType.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/override/ExtendedCreateIndex.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateIndexCommand.java
Show resolved
Hide resolved
|
|
||
| /** | ||
| * An extension of the {@link DefaultCreateIndex} class, This is needed because the column name | ||
| * appended to the builder needs to use `asCql(true)` to keep the quotes. |
There was a problem hiding this comment.
Is this really true? Can the caller not pass properly constructed CqlIdentifier to ensure this?
And if not, is this a bug?
EDIT: base class does have things as CqlIdentifier so it looks like a bug to report. And if so, we are doing override in the meantime so as not to be blocked.
If so, makes sense.
There was a problem hiding this comment.
Yes., reported this to drivers team.
src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/CreateIndexAttempt.java
Outdated
Show resolved
Hide resolved
| TextIndexOptions textIndexOptions, | ||
| VectorIndexOptions vectorIndexOptions, | ||
| boolean ifNotExists) { | ||
| super(position, schemaObject, new SchemaRetryPolicy(2, Duration.ofMillis(10))); |
There was a problem hiding this comment.
Is the idea that we'll always retry? Even with ifNotExists being false?
There was a problem hiding this comment.
TBH, we didn't spec ifNotExists but did it since we have support in query builder. If it's creating confusion should I remove this feature?
There was a problem hiding this comment.
May be worth discussing: we should probably always enable "IF NOT EXISTS" for idempotent operation.
But since we already have that for CreateTableCommand, maybe it makes sense to expose it.
There was a problem hiding this comment.
That's a good catch, not sure what it means to retry with if not exists is false. In other words what to do on timeouts.
src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/CreateIndexAttempt.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/CreateIndexAttempt.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/CreateIndexAttemptBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/CreateIndexAttemptBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateIndexCommandResolver.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/resolver/CreateIndexCommandResolver.java
Outdated
Show resolved
Hide resolved
| similarityFunction = SimilarityFunction.COSINE; | ||
| } | ||
|
|
||
| var attempt = |
There was a problem hiding this comment.
Ok: so since pretty much everything is already available, not sure there's even need for Builder is there? Or if there is, should at least pass more of settings (at least "columnName", "indexName", "ifNotExists") to constructor.
src/main/java/io/stargate/sgv2/jsonapi/service/schema/SimilarityFunction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good overall, but have some questions/suggestions. Important ones:
- Handling of identifiers: should probably more eagerly construct
CqlIdentifiers - Operation attempt builder: should pass more of settings to constructor -- or even eliminate builder altogether -- since it seems all settings are available by the time builder is constructed
- Questions on bugs in Java driver: overrides are probably fine but wanted to make sure these are just work-arounds (esp. wrt use of
CqlIdentifier)
Will change the constructor to use CqlIdentifier.
They are reported to driver team, these are work around until fixes are available.
Agree, Atleast for this will have the Builders do the CqlIdentifier conversion. Just trying to stick with the template how other commands are done. |
I am not sure there is value in making builders more flexible than we need them -- or to even use builders, if they are not needed. They really should be used for optional parts, not for things that are always required. So that should be the guideline: mandatory properties should be passed in constructors, optional ones via builder methods. EDIT: looks like builder is changed to have most things immutable, that works. |
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist