Skip to content
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.

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

Implementing keyspace qualification for particular entities #1400

Closed
wants to merge 3 commits into from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Jun 26, 2023

Hello @mp911de @schauder !
I have implemented #921 feature. I have assumed the following behavoir:

  1. There is a possibility to specify a keyspace for an entity in the @Table annotation. If it is specified, then for each query we just pass to QueryBuilder keyspace specified by user. The goal again is not to mutate CqlSession and avoid creating CqlSession for each keyspace.
  2. On the other hand, if keyspace is omitted or empty, we assume the entity's corresponding table is in the default for current CqlSession namespace.

I hope it makes sence to you as well. Any suggestions/improvements will be highly appretiated. Thanks!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 26, 2023
@mp911de
Copy link
Member

mp911de commented Jun 29, 2023

Have you seen #179? Introducing keyspace to tables raises another set of questions that we tried to address with the previous pull request. For simplicity, let's decouple all polishing within this pull request into a separate one, as the cleanups are almost a no-brainer to merge.

@mipo256
Copy link
Contributor Author

mipo256 commented Jun 29, 2023

@mp911de No, not yet. But I have checked it now.

  1. DATACASS-751: Allow keyspace customization for CassandraPersistentEntity #179 (review)

Regardign this commet, I think it is the good idea. So we'll just introduce new getTableInfo() method that would return a complex object with keyspace and table name. I guess then we need to mark getTableName() as @Deprecated since accessign table name without keyspace actually loses the sense.

P.S: Also I think we should not expose setTableName() to be honest. Ideally, it should be immutable and passed to Cassandra PersistentEntity by the time of it's creation.

2.#179 (comment)

I can introduce new overloaded methods to an API of CassandraTemplate to accept this new TableCoordinates, no problem.

Any other key components for me to consider?)

By the way, I have separated the polishing commits already. You want me to create a separate PR for this?)

@mp911de mp911de self-assigned this Jul 3, 2023
@mp911de
Copy link
Member

mp911de commented Jul 11, 2023

Applying a keyspace to a table sounds straight-forward, but there are a few things to consider:

  1. CassandraTemplate and the underlying CqlTemplate operate within a single keyspace as CqlSession is bound to a keyspace. This complicates schema support as we create a table/user type within that keyspace. All of our specifications (e.g. AlterTableSpecification, CreateTableSpecification) would require another keyspace property to define a different keyspace to generate the appropriate DML statements.
  2. Tables using user-defined types from different keyspaces would require an appropriate resolution mechanism. UserTypeResolver accepts a name only as it is tied to a single keyspace.

IMO, the keyspace isn't something that is detached from the actual identifier but it rather enriches it. A much more appropriate way to think about this is thinking whether the identifier is qualified or unqualified and having an extension to CqlIdentifier in the sense of QualifiedCqlIdentifier. Such an approach could allow not to introduce of additional API but rather consider qualified identifiers in e.g. SimpleUserTypeResolver, the CQL generator, or our StatementFactory. For this to be possible, CqlIdentifier's constructor must be protected to allow subclasses.

@mp911de
Copy link
Member

mp911de commented Jul 11, 2023

I filed https://datastax-oss.atlassian.net/browse/JAVA-3088 asking for constructor rules relaxation.

@mipo256
Copy link
Contributor Author

mipo256 commented Jul 15, 2023

Yeah, I agree with you.
I also think we should think of keyspace and table name as a single identifier of the table in cassandra cluster. For this I have created the PR in datastax java driver: apache/cassandra-java-driver#1683

@mp911de mp911de linked an issue Jul 25, 2024 that may be closed by this pull request
@mp911de mp911de added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 25, 2024
@pivotal-cla
Copy link

@mipo256 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

1 similar comment
@pivotal-cla
Copy link

@mipo256 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@mp911de
Copy link
Member

mp911de commented Jul 25, 2024

Superseded as per #921 (comment). Thank you for your contribution to the design of customizing keyspaces.

@mp911de mp911de closed this Jul 25, 2024
mp911de added a commit that referenced this pull request Jul 25, 2024
We now allow setting the keyspace on `@Table` and `@UserDefinedType` to interact with tables and types residing in a specific keyspace.

@table(keyspace = "some_ks")
class Person {
 …
}

Keyspace values can be either string literals or value expressions (#{…}, ${…}) to use the context or config properties to compute the actual keyspace.

Special honors go to @tomekl007 and @mipo256 for their contributions.

See #1400
See #279

Closes #921
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow keyspace customization for CassandraPersistentEntity
4 participants