refactor(framework) Migrate ID handling from sint64 to uint64 #4170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mohammadnaseri
changed the title
mohammadnaseri
requested review from
danieljanes and
tanertopal
as code owners
mohammadnaseri
changed the title
mohammadnaseri
changed the title
chongshenng
previously requested changes
Sep 12, 2024
There was a problem hiding this comment.
Overall looks fine to me. Minor comment about whether we should use parameterize for the test cases. I'm unfamiliar with sqlite_state.py so I did not review it. Perhaps @panh99 can give feedback there?
panh99
requested changes
Sep 12, 2024
There was a problem hiding this comment.
It looks good to me generally. However, I am a bit concerned about how users gonna send/receive these IDs. For now, our major grpc messages like Scalar and ConfigsRecordValue do not support uint64. They only support sint64. This might prevent our users from storing IDs, e.g., node_id, in grpc messages. We may need to update Scalar, ConfigsRecordValue definitions and their serialization functions in serde.py accordingly.
panh99
reviewed
Sep 12, 2024
danieljanes
reviewed
Sep 12, 2024
danieljanes
reviewed
Sep 12, 2024
panh99
previously approved these changes
Sep 12, 2024
danieljanes
reviewed
Sep 17, 2024
danieljanes
changed the title
danieljanes
reviewed
Sep 18, 2024
danieljanes
reviewed
Sep 18, 2024
danieljanes
approved these changes
Sep 18, 2024
chongshenng
pushed a commit
that referenced
this pull request
Sep 19, 2024
Co-authored-by: Heng Pan <pan@flower.ai> Co-authored-by: Daniel J. Beutel <daniel@flower.ai>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the ID generation to use
uint64. To ensure compatibility with SQLite, which usessint64, the IDs are converted as follows:During Save: Convert
uint64IDs tosint64before storing in SQLite.During Read: Convert
sint64IDs back touint64after retrieving from SQLite.