-
Notifications
You must be signed in to change notification settings - Fork 2k
Fixed the issue with executing the saveAll method in JdbcChatMemoryRepository #3154
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
Conversation
…All would update the timestamps of all historical messages to the current timestamp. Signed-off-by: Sun Yuhan <[email protected]> Signed-off-by: Sun Yuhan <[email protected]>
Signed-off-by: Sun Yuhan <[email protected]>
2d2f042 to
b22c038
Compare
|
Hi @ThomasVitale |
|
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 |
|
So developers have to maintain additional chat record tables? |
|
I think that, as of now, Spring AI has not yet implemented a feature specifically for "historical messages." At one point, I thought that I'm also very interested in learning about any future plans regarding support for "historical messages." |
@sunyuhan1998 However, in sql, it is clearly stated that timestamp is a timestamp. And this leads to a disordered message order and also affects the output of the llm. |
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. |
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.