Skip to content

Migrate CassandraChatMemory to CassandraChatMemoryRepository #2998

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

Closed
markpollack opened this issue May 5, 2025 · 15 comments
Closed

Migrate CassandraChatMemory to CassandraChatMemoryRepository #2998

markpollack opened this issue May 5, 2025 · 15 comments
Assignees
Milestone

Comments

@markpollack
Copy link
Member

This would allow us to have a more robust implementation than we currently have based on the ChatMemory.get(conversationId, lastn) interface that is currently deprecated.

This has been done for Jdbc in the PR - as a basis for comparison.

#2890

@linarkou
Copy link
Contributor

linarkou commented May 6, 2025

As for now, cassandraChatMemory stores data in table with 4 columns like that:

create table ai_chat_memory (
    session_id        text,
    message_timestamp timestamp,
    assistant         text,
    user              text,
    primary key (session_id, message_timestamp)
)
    with clustering order by (message_timestamp desc);

Maybe we should change table structure similar to JdbcChatMemory?

create table ai_chat_memory (
    session_id        text,
    message_timestamp timestamp,
    type              text,
    content           text,
    primary key (session_id, message_timestamp)
)
    with clustering order by (message_timestamp desc);

It'll give us an ability to store tool/system messages, not only user/assistant.

What do you think @markpollack @michaelsembwever ?

@michaelsembwever
Copy link
Contributor

Maybe we should change table structure similar to JdbcChatMemory?

Then the table would need a primary key like

primary key (session_id, message_timestamp, type)

Which can be done, and I have nothing against such an approach.

@linarkou
Copy link
Contributor

linarkou commented May 7, 2025

Can't session_id+message_timestamp be a primary key? As far as I remember you set unique timestamp.

On other hand, there is #2902 with request on message id field, so we can introduce it here.

@michaelsembwever
Copy link
Contributor

@linarkou , let's create a separate issue for your suggestion, as there's a time-constraint to get this issue fixed.

@michaelsembwever
Copy link
Contributor

@markpollack draft PR @ #3036
eta to finish it tomorrow.

michaelsembwever added a commit to michaelsembwever/spring-ai that referenced this issue May 8, 2025
michaelsembwever added a commit to michaelsembwever/spring-ai that referenced this issue May 8, 2025
michaelsembwever added a commit to michaelsembwever/spring-ai that referenced this issue May 8, 2025
michaelsembwever added a commit to michaelsembwever/spring-ai that referenced this issue May 8, 2025
@michaelsembwever
Copy link
Contributor

#3036 is ready for review now.

michaelsembwever added a commit to michaelsembwever/spring-ai that referenced this issue May 9, 2025
@markpollack
Copy link
Member Author

fantastic!

@markpollack markpollack self-assigned this May 9, 2025
markpollack pushed a commit that referenced this issue May 9, 2025
@michaelsembwever
Copy link
Contributor

There's a bug here I need to fix. @linarkou statement above

It'll give us an ability to store tool/system messages

is actually mandatory… MessageWindowChatMemory relies on persisting SystemMessages.
working on a fix…

@markpollack
Copy link
Member Author

Thanks @michaelsembwever

@michaelsembwever
Copy link
Contributor

bug (optimisation) fix in progress: #3097
completion eta ~24 hours.

@markpollack
Copy link
Member Author

@michaelsembwever thanks eh!

@markpollack
Copy link
Member Author

@michaelsembwever we can always do a bug fix post RC1 pre GA - just want to make sure we avoid modifying and public facing apis/properties.

@michaelsembwever
Copy link
Contributor

@markpollack , ready for review #3097

@michaelsembwever
Copy link
Contributor

michaelsembwever commented May 11, 2025

we can always do a bug fix post RC1 pre GA

i don't think before #3097 it would work at all with MessageWindowChatMemory (as it expected to persist SystemMessages).

and just generally, the usage pattern of MessageWindowChatMemory was substantially different: and with Cassandra not being a relationship style db; that it really required a revamp of the schema. #3097 is properly aligned to Cassandra now, provides a number of advantages over other chat-memory implementations (according to its known strengths), and could quite likely outperform latency-wise the other implementations (ofc "it depends").

@markpollack
Copy link
Member Author

Thanks so much for the deep analysis and changes at the last minute.

If you want to advertise this in the docs, it may draw more folks into using cassandra.

merged in 87b680a

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

No branches or pull requests

3 participants