Skip to content

Conversation

sunyuhan1998
Copy link
Contributor

In JdbcChatMemoryRepository, when the saveAll method is executed, it deletes all historical messages in the current conversation and saves the current message list to the database. However, during this process, it updates the timestamp of all Messages to the current time, which causes the timestamps of historical messages to be incorrect.

This PR resolves the issue by storing the timestamp of each Message in its metadata when the findByConversationId method is called. Then, during the saveAll operation, it checks whether each Message's metadata contains a timestamp. If a timestamp exists, it retains the original time; otherwise, it initializes the timestamp to the current time.

…All would update the timestamps of all historical messages to the current timestamp. Signed-off-by: Sun Yuhan <sunyuhan1998@qq.com> Signed-off-by: Sun Yuhan <1085481446@qq.com>
Signed-off-by: Sun Yuhan <1085481446@qq.com>
@sunyuhan1998
Copy link
Contributor Author

Hi @ThomasVitale
What do you think about this?

@markpollack markpollack self-assigned this May 14, 2025
@markpollack markpollack added this to the 1.0.0 milestone May 14, 2025
@markpollack
Copy link
Member

The timestamp is really a sequence id to preserve order. We probably should have renamed the field in the schema as it was a holder over of the previous support which was storing messages historically and then picking a subset.

This instead is being used not for historical list of messages, but for a block of messages as a group. If we want to keep history of messages, we can address that in a different feature set.

@sunyuhan1998
Copy link
Contributor Author

The timestamp is really a sequence id to preserve order. We probably should have renamed the field in the schema as it was a holder over of the previous support which was storing messages historically and then picking a subset.

This instead is being used not for historical list of messages, but for a block of messages as a group. If we want to keep history of messages, we can address that in a different feature set.

Oh, I see! Thank you for the explanation. I was previously quite confused about why the content of the timestamp field was implemented through auto-incrementing, but now everything makes sense. The current table only records a portion of the conversation window, used as context for the model—it’s not actually a "message history log." Given this, yes, the timestamp field really does need to be renamed. Judging from some of the issues raised, this has indeed already caused confusion among users.

@xjh1994
Copy link

xjh1994 commented May 20, 2025

So developers have to maintain additional chat record tables?

@sunyuhan1998
Copy link
Contributor Author

I think that, as of now, Spring AI has not yet implemented a feature specifically for "historical messages." At one point, I thought that ChatMemory was intended for storing historical message data, but the introduction of MessageWindowChatMemory seems to have changed that understanding. MessageWindowChatMemory only retains a limited number of recent messages and discards older ones when the limit is reached.

I'm also very interested in learning about any future plans regarding support for "historical messages."
What are your thoughts on this? @markpollack @ThomasVitale

@xjh1994
Copy link

xjh1994 commented May 20, 2025

I think that, as of now, Spring AI has not yet implemented a feature specifically for "historical messages." At one point, I thought that ChatMemory was intended for storing historical message data, but the introduction of MessageWindowChatMemory seems to have changed that understanding. MessageWindowChatMemory only retains a limited number of recent messages and discards older ones when the limit is reached.

I'm also very interested in learning about any future plans regarding support for "historical messages." What are your thoughts on this? @markpollack @ThomasVitale

@sunyuhan1998 However, in sql, it is clearly stated that timestamp is a timestamp.

CREATE TABLE IF NOT EXISTS SPRING_AI_CHAT_MEMORY ( conversation_id VARCHAR(36) NOT NULL, content TEXT NOT NULL, type VARCHAR(10) NOT NULL, `timestamp` **TIMESTAMP** NOT NULL, CONSTRAINT TYPE_CHECK CHECK (type IN ('USER', 'ASSISTANT', 'SYSTEM', 'TOOL')) ); 

And this leads to a disordered message order and also affects the output of the llm.

@sunyuhan1998
Copy link
Contributor Author

it is clearly stated that timestamp is a timestamp.

I understand that according to @markpollack , this situation likely has some historical reasons. Perhaps in the early stages, this field was indeed a timestamp, but over time its purpose evolved into an auto-incrementing sequence. However, the name of this field and its corresponding comment in the SQL were never updated accordingly.

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