-
Notifications
You must be signed in to change notification settings - Fork 2
Index strings #41
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
Index strings #41
Conversation
|
/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 string indexing mechanism to optimize database storage by replacing repeated strings with integer IDs. A new T_STRINGS table and a GTFSStringsUtils helper class are added to manage this. The changes are well-implemented, using a feature flag for a controlled rollout. My review includes a couple of suggestions to improve code clarity and maintainability by using modern language features and removing redundancy.
src/main/java/org/mtransit/android/commons/provider/GTFSProviderDbHelper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mtransit/android/commons/provider/GTFSStringsUtils.kt
Outdated
Show resolved
Hide resolved
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 an "Index Strings" feature that optimizes storage by replacing repeated string values with integer identifiers. The changes introduce a new STRINGS table and supporting infrastructure to handle string indexing across GTFS data tables.
Key Changes
- Added
GTFSStringsUtilsutility class to manage string indexing operations including database loading and string replacement - Integrated string replacement logic into database initialization and schedule timestamp processing
- Refactored return types from
HashSettoSetinterface for better flexibility
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| GTFSStringsUtils.kt | New utility class providing string indexing operations including database queries, file parsing, and string replacement functionality |
| GTFSStatusProvider.java | Updated to use Set interface and integrated string replacement in schedule loading when feature flag is enabled |
| GTFSScheduleTimestampsProvider.java | Changed from HashSet to Set interface for consistency |
| GTFSRDSProvider.java | Renamed constant from TAG to LOG_TAG for consistency and made cursor final |
| GTFSProviderDbHelper.java | Major changes to support strings table including new initialization logic, string file reading, and integration of string replacement during table initialization |
| GTFSPOIProvider.java | Renamed constant from TAG to LOG_TAG for consistency |
| Schedule.java | Added discouraged accessor methods for direct headsign value manipulation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/mtransit/android/commons/provider/GTFSProviderDbHelper.java
Outdated
Show resolved
Hide resolved
|
/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 string indexing mechanism to optimize data storage, which is a solid improvement. The changes involve creating a new strings table in the database, pre-processing data files to replace strings with IDs during DB creation, and resolving these IDs back to strings at runtime. The implementation is well-structured, with a new GTFSStringsUtils class for handling string operations and modifications to the GTFSProviderDbHelper for database setup. I've found one area for improvement regarding error handling in the new utility class to make it more robust against malformed data files. Overall, this is a good step towards optimizing the application's data layer.
|
/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 reduce the database size by indexing repeating strings, which is a great improvement. The implementation is mostly solid, with new Kotlin utility classes for handling string lookups and database initialization.
My review focuses on a few key areas:
- A significant performance issue in
GTFSStatusProviderwhere database queries are made inside a loop. - Several opportunities to improve code readability and maintainability in the new Kotlin files and the database helper.
- A minor bug in a log message.
I've provided detailed comments and suggestions for each of these points.
Uh oh!
There was an error while loading. Please reload this page.