-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: #2883 pickle data was truncated error in database session using MySql #2884
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @Lin-Nikaido, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves a critical data truncation issue affecting the storage of pickled objects in MySQL database sessions. By adjusting the data type mapping for MySQL, it ensures that larger serialized data can be correctly persisted and retrieved, preventing unpickling errors and data loss.
Highlights
- Fix for pickle data truncation with MySQL: The
DynamicPickleType
was causing_pickle.UnpicklingError
when large data was stored in MySQL sessions because it defaulted toBLOB
, which has a smaller storage capacity. This change explicitly configuresDynamicPickleType
to useLONGBLOB
for MySQL dialects, ensuring proper storage and retrieval of large pickled objects.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This change correctly addresses the _pickle.UnpicklingError
on MySQL by using LONGBLOB
instead of the default BLOB
for DynamicPickleType
. The implementation is consistent with how other dynamic types like DynamicJSON
are handled in this file, and it effectively resolves the data truncation issue.
Regarding your question about testing: to test this database-specific change, you would typically need an integration test that runs against a MySQL database. The existing unit tests in tests/unittests/sessions/test_session_service.py
use an in-memory SQLite database, which is why you couldn't find a suitable place.
For a new test, you could:
- Set up a test environment that can connect to a MySQL instance (e.g., using Docker for local testing or a dedicated test DB in CI).
- Create a new test file, perhaps in the
tests/integration/
directory. - In that test, initialize
DatabaseSessionService
with a MySQL connection URL. - Write a test case that creates a session and appends an event with a large
actions
payload that would exceed the size limit ofBLOB
but fit intoLONGBLOB
. - Assert that the event can be appended and retrieved successfully without a
UnpicklingError
or data truncation error.
This would ensure the fix works as expected and protects against future regressions. Since setting this up might be outside the scope of this small fix, it could also be handled in a follow-up task.
The change looks good to merge.
Response from ADK Triaging Agent Hello @Lin-Nikaido, thank you for submitting this pull request! To help us review it more effectively, could you please add a Thank you! |
closes: #2883
Fix
When put leage data into event and load it. the _pickle.UnpicklingError was occurred.
The root caurse is
DynamicPickleType
mappingBLOB
as default in case of MySql, notLONGBLOB
. And learge data will be able to cut off tail of data. And raise pickle error.What todo
Defined
LONFBLOB
as default explicitly.Question
Where should we code the test code like this case? I cannot found the test code the DB and table was created expectedly.