-
Notifications
You must be signed in to change notification settings - Fork 3
Index Service IDs #18
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
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.
Pull Request Overview
This PR implements service ID indexing to reduce the size of exported MClasses data by replacing service ID strings with integer indices. Service IDs are mapped to integers and stored in a separate file, allowing schedules and other data to reference the smaller integer values instead of full string IDs.
Key changes:
- Introduces
MServiceIdssingleton to manage service ID to integer mappings - Adds
MServiceIddata class for serialization/deserialization - Updates data export logic to conditionally use integer IDs based on feature flag
- Refactors property accessors to use expression body syntax for consistency
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| MServiceIds.kt | New singleton managing service ID to integer mappings with bidirectional lookup |
| MServiceId.kt | New data class for service ID persistence with file serialization support |
| MServiceDate.kt | Updated to export service ID integers when feature flag enabled |
| MSchedule.kt | Updated to export service ID integers when feature flag enabled |
| MFrequency.kt | Updated to export service ID integers when feature flag enabled |
| MAgency.kt | Refactored property accessor to expression body syntax |
| MReader.kt | Added loadServiceIds method to read service IDs from file |
| MGenerator.java | Added service IDs export logic and cleanup regex configuration |
| MDataChangedManager.kt | Removed unnecessary suppression annotation |
| GenerateMObjectsTask.java | Simplified service date creation using factory method |
| GTripStop.kt | Refactored property accessors to expression body syntax |
| GTrip.kt | Refactored property accessors to expression body syntax and removed unused method |
| GStopTime.kt | Refactored property accessors to expression body syntax |
| GStop.kt | Refactored property accessors to expression body syntax |
| GSpecExt.kt | Simplified function to expression body syntax |
| GRoute.kt | Refactored property accessors to expression body syntax |
| GFrequency.kt | Refactored property accessors to expression body syntax |
| GDirection.kt | Refactored property accessors to expression body syntax |
| GCalendarDate.kt | Refactored property accessors and removed unused method |
| GCalendar.kt | Refactored property accessors, removed unused method, and fixed default exception type |
| GAgency.kt | Refactored property accessor to expression body syntax |
| DumpDbUtils.kt | Added service IDs table creation/drop when feature flag enabled |
| DefaultAgencyTools.java | Added service IDs loading and initialization during startup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/gemini review |
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 pull request introduces an optimization to index service IDs, replacing string representations with integers to reduce the size of exported data. The changes are well-structured, using a feature flag to control the new behavior and a dedicated MServiceIds singleton for managing the ID mapping. The implementation is thread-safe, which is crucial given the concurrent processing in MGenerator. I've also noticed several nice Kotlin refactorings that improve code style and readability. I found one minor issue with logging in MReader.kt where a 'file not found' message would never be displayed.
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 pull request introduces service ID indexing to reduce the exported data size, which is a valuable optimization. The implementation is well-structured, introducing MServiceId and MServiceIds to manage the indexed IDs, and the changes are cleanly isolated behind a feature flag. The widespread refactoring of Kotlin data classes to use more idiomatic property getters is also a great improvement to code style and readability.
I have a couple of minor suggestions to enhance the code further:
- In
MGenerator.java, using atry-with-resourcesstatement for file writing would make the code safer and more concise. - In
MReader.kt, the log message for a missing file could be improved for better debugging.
Overall, this is a high-quality contribution that effectively achieves its goal.
|
/gemini review |
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 pull request introduces a significant optimization by indexing service IDs to reduce the exported data size. The changes are well-structured and include numerous beneficial refactorings that improve code style and maintainability, such as adopting more idiomatic Kotlin and centralizing logic. However, I've identified a critical race condition in the new MServiceIds class that could lead to data corruption in a multi-threaded environment. Additionally, there are a few places where removing comments has slightly reduced code readability. Overall, this is a valuable change, and addressing these points will make it even more robust.
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.
Pull Request Overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The priority here is to make the exported data (MClasses) small[er].
(service IDs are tricky because they are not "cleaned" while reading the data to avoid override issue)
The next steps would indexing words from all the user facing strings...