refactor: add support for new timestamp column name in datasets
#2237
Conversation
jo-elimu
requested review from
AshishBagdane,
nya-elimu and
tomaszsmy
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2237 +/- ##
============================================
+ Coverage 16.71% 16.87% +0.16%
Complexity 423 423
============================================
Files 245 245
Lines 6815 6838 +23
Branches 789 793 +4
============================================
+ Hits 1139 1154 +15
- Misses 5626 5630 +4
- Partials 50 54 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe extraction methods in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CsvAnalyticsExtractionHelper
Caller->>CsvAnalyticsExtractionHelper: extract<Event>FromCsv(csvFile)
CsvAnalyticsExtractionHelper->>CsvAnalyticsExtractionHelper: extract versionCode from filename
CsvAnalyticsExtractionHelper->>CsvAnalyticsExtractionHelper: Determine timestamp column ("time" or "timestamp")
CsvAnalyticsExtractionHelper->>CsvAnalyticsExtractionHelper: Extract event fields using correct column
CsvAnalyticsExtractionHelper-->>Caller: Return extracted event
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java (1)
src/main/java/ai/elimu/util/AnalyticsHelper.java (1)
AnalyticsHelper(6-43)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
🔇 Additional comments (2)
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java (2)
36-37: LGTM: Version code extraction and logging added.The version code extraction and logging provide good traceability for debugging CSV processing issues.
257-259:Details
❌ Incorrect review comment
Logic appears inverted - verify the intended behavior.
The current logic sets an empty string for versions < 3003000 and reads from CSV for versions >= 3003000. This suggests that the
storybook_titlecolumn was added in version 3.3.0. Please verify this is the intended behavior as it differs from the typical pattern of handling missing columns.
🏁 Script executed:
#!/bin/bash # Description: Verify when the storybook_title column was introduced # Expected: Find references to version 3.3.0 and storybook_title column introduction # Search for storybook_title column references and version information rg -A 5 -B 5 "storybook_title|3003000|3\.3\.0" # Check if there are any tests or documentation about this change fd -e md -e txt | xargs rg -l "storybook.*title|3\.3\.0"Length of output: 7688
Logic is correct; no inversion needed
The
storybook_titlecolumn was introduced in version 3.3.0 (3003000), so for all older versions it must be blank and only parsed for versionCode >= 3003000. This is confirmed by:
- The in-code comment referencing https://github.com/elimu-ai/analytics/releases/tag/3.3.0
- CSV export controller and JSP templates including
storybook_titleonly in newer formats- Existing test
testExtractStoryBookLearningEvents_v3003000validating extraction for version 3003000No changes are required here.
Likely an incorrect or invalid review comment.
| String timestampColumnName = null; | ||
| if (versionCode < 3004000) { | ||
| timestampColumnName = "time"; | ||
| } else { | ||
| // https://github.com/elimu-ai/analytics/releases/tag/3.4.0 | ||
| timestampColumnName = "timestamp"; | ||
| } | ||
| long timestampInMillis = Long.valueOf(csvRecord.get(timestampColumnName)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Approve the version-based timestamp column logic.
The implementation correctly handles the schema change in version 3.4.0. However, this is a duplicate of the same pattern used in other methods.
🤖 Prompt for AI Agents
In src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java around
lines 239 to 246, the version-based logic for selecting the timestamp column
name is correctly implemented but duplicated in other methods. Refactor by
extracting this logic into a single reusable private method that returns the
appropriate timestamp column name based on the versionCode, and replace all
duplicate instances with calls to this new method to improve maintainability.
Issue Number
time-->timestamp) #2101Purpose
Technical Details
Testing Instructions
Screenshots
Format Checks
Note
Files in PRs are automatically checked for format violations with
mvn spotless:check.If this PR contains files with format violations, run
mvn spotless:applyto fix them.