Cassandra vector store builder refactoring #1912

sobychacko · 2024-12-12T02:27:59Z

Deprecate CassandraVectorStoreConfig
Move its contents to a top level Builder class within the CassandraVectorStore

Restructuring packages for Cassandra vector store and chat memory classes

- Deprecate CassandraVectorStoreConfig - Move its contents to a top level Builder class within the CassandraVectorStore Restructuring packages for Cassandra vector store and chat memory classes

sobychacko · 2024-12-12T15:36:01Z

@michaelsembwever We are trying to make the vector store builder pattern consistent. See the recently merged chroma vector store for a template. Cassandra vector store seems to be more involved. Since you are the original author of the cassandra vector store, can you take a look at this PR and see what you think? Thank you!

michaelsembwever · 2024-12-12T21:22:11Z

...ngframework/ai/autoconfigure/chat/memory/cassandra/CassandraChatMemoryAutoConfiguration.java


-import org.springframework.ai.chat.memory.CassandraChatMemory;
-import org.springframework.ai.chat.memory.CassandraChatMemoryConfig;
+import org.springframework.ai.chat.memory.cassandra.CassandraChatMemory;


are all vectorstore modules moving their classes to subpackages ? (i don't see that yet with pgvector. has there been some discussion/agreement to this somewhere we can link to ?)

Yes, we are in the process of moving the vector store classes under that structure - in this case - org.springframework.ai.chat.memory.cassandra and springframework.ai.vectorstore.cassandra. The issue was that the vector store classes currently use the same package name in all the different modules - springframework.ai.vectorstore.

was hoping that there was some link you could provide to where the discussion had been had and landed on that conclusion and action. but it's ok, i trust.

next time we will reach out to you, i wish we had a more centralized way to communicate changes to frequent contributors. we are working on this.

michaelsembwever · 2024-12-12T21:23:25Z

...a-store/src/main/java/org/springframework/ai/vectorstore/cassandra/CassandraVectorStore.java

i can't read the diff here (because the change is clobbering the move).
would it be possible to do the move in a separate commit, to make the reviewing simpler…?

Does this link work? https://github.com/spring-projects/spring-ai/blob/92b420c4a9c70ebf4b460c209706d2c9d7fbdcc5/vector-stores/spring-ai-cassandra-store/src/main/java/org/springframework/ai/vectorstore/cassandra/CassandraVectorStore.java

i don't see any diff there^ 😔

michaelsembwever · 2024-12-12T21:25:15Z

...a-store/src/main/java/org/springframework/ai/vectorstore/cassandra/CassandraVectorStore.java

+	 * https://github.com/apache/cassandra-java-driver/tree/4.x/manual/core/configuration
+	 *
+	 */
+	public static class CassandraBuilder extends AbstractVectorStoreBuilder<CassandraBuilder> {


does CassandraBuilder have to be an inner class ?
no strong opinion, but i gut instinct is it's big enough to warrant being a separate java file.

We can consider that - but wanted to make the usage consistent across all the vector stores.

i would suggest that, given the lines of code and that the builder has a little more logic in it the a super simple builder. (i had previously intentionally kept the classes separate for the sake of encapsulation between startup/configuration versus runtime behaviour).

it's not a hill i will die on. if you take past PoVs into account, then i'm happy it's your call to make.

michaelsembwever · 2024-12-12T21:26:23Z

...test/java/org/springframework/ai/vectorstore/cassandra/CassandraRichSchemaVectorStoreIT.java

 				String[] parts = id.split("§¶");
 				String title = parts[0];
-				int chunk_no = 0 < parts.length ? Integer.parseInt(parts[1]) : 0;
+				int chunk_no = parts.length > 0 ? Integer.parseInt(parts[1]) : 0;


why this change ?

I will revert that.

michaelsembwever · 2024-12-12T21:29:48Z

...store/src/test/java/org/springframework/ai/vectorstore/cassandra/WikiVectorStoreExample.java

 					String[] parts = id.split("§¶");
 					String title = parts[0];
-					int chunk_no = 0 < parts.length ? Integer.parseInt(parts[1]) : 0;
+					int chunk_no = parts.length > 0 ? Integer.parseInt(parts[1]) : 0;


I will revert that.

michaelsembwever · 2024-12-12T21:32:45Z

...gframework/ai/autoconfigure/vectorstore/cassandra/CassandraVectorStoreAutoConfiguration.java

+			.disallowSchemaChanges(!properties.isInitializeSchema())
+			.returnEmbeddings(properties.getReturnEmbeddings())


there's a behaviour change here, in that we're no longer respecting the builder's default, imposing the autoConfiguration's default over it. (the builder's default should be canonical)

i'm not sure how important this is… 🤷

Good catch, we should be consistent. I'll create a new issue to capture this and discuss.

michaelsembwever · 2024-12-12T21:38:17Z

@sobychacko the overall direction of the refactoring looks good to me, nice work.

Threw in a few questions. The reviewing need to separate config-build vs package renaming is the big one.
Otherwise, not having kept up with all project discussions and changes, I'm missing the project wide statement/agreement to move to this builder pattern and packaging naming. My concern would be that we're only applying this to Chroma and Cassandra. I assume that's not the case, but would still like an explicit reference link to this agreement :)

sobychacko · 2024-12-14T01:23:57Z

@michaelsembwever Yes, we are moving to the builder pattern for all the vector store classes to make it consistent. Started with chroma,and there are a few PR's out there that's performing this migration on the other vector stores. Before the M5 release, we hope to complete this migration.

markpollack · 2024-12-16T20:55:29Z

Thanks for your patience and understanding @michaelsembwever

I've created #1949 to track the remaining comment in the discussion.

I've merged this after updating ref doc and class javadoc.

Merged in 25123a5

sobychacko · 2025-05-12T15:04:13Z

@michaelsembwever linking the original epic here: #1825

sobychacko added 2 commits December 11, 2024 21:26

Cassandra vector store builder refactoring

228056e

- Deprecate CassandraVectorStoreConfig - Move its contents to a top level Builder class within the CassandraVectorStore Restructuring packages for Cassandra vector store and chat memory classes

Removing the usage of the config class

92b420c

michaelsembwever reviewed Dec 12, 2024

View reviewed changes

Addressing PR review

782ec0d

markpollack mentioned this pull request Dec 16, 2024

Investigate consistency in Cassandra Autoconfiguration and Builder #1949

Closed

markpollack added cassandra vector store labels Dec 16, 2024

markpollack closed this Dec 16, 2024

		.disallowSchemaChanges(!properties.isInitializeSchema())
		.returnEmbeddings(properties.getReturnEmbeddings())

Cassandra vector store builder refactoring #1912

Cassandra vector store builder refactoring #1912

Uh oh!

Conversation

sobychacko commented Dec 12, 2024

Uh oh!

sobychacko commented Dec 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelsembwever commented Dec 12, 2024

Uh oh!

sobychacko commented Dec 14, 2024

Uh oh!

markpollack commented Dec 16, 2024

Uh oh!

sobychacko commented May 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants