fix: handle change to collectDoubles in convertTileDriftSeriesInfoToTimeSeries #293
Conversation
WalkthroughThe pull request enhances null handling in the Changes
Possibly related PRs
Suggested Reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
ken-zlai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hub/src/main/scala/ai/chronon/hub/handlers/TimeSeriesHandler.scala (1)
126-132: LGTM! Safe handling of null lists.The changes correctly handle null cases by returning empty sequences.
Minor formatting suggestion:
val lhsList = if (metric == NullMetric) { Option(tileDriftSeries.nullRatioChangePercentSeries).map(_.asScala).getOrElse(Seq.empty) } else { - if (isNumeric) - Option(tileDriftSeries.percentileDriftSeries).map(_.asScala).getOrElse(Seq.empty) - else - Option(tileDriftSeries.histogramDriftSeries).map(_.asScala).getOrElse(Seq.empty) + if (isNumeric) Option(tileDriftSeries.percentileDriftSeries).map(_.asScala).getOrElse(Seq.empty) + else Option(tileDriftSeries.histogramDriftSeries).map(_.asScala).getOrElse(Seq.empty) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
hub/src/main/scala/ai/chronon/hub/handlers/TimeSeriesHandler.scala(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (1)
hub/src/main/scala/ai/chronon/hub/handlers/TimeSeriesHandler.scala (1)
122-124: LGTM! Safer null handling with Option.
|
Interestingly, I pulled main and re-ran
the charts rendered without an issue (as the second
Checking out your PR now and restarted the docker environment, but wanted to toss that out there (and in case I'm looking at it wrong) |
|
I'm absolutely dumbfounded here - I rebuilt everything from main this morning and our charts work as expected. I'll close this PR since things seem to be working. I really don't know why I've seen inconsistent behavior - maybe a rouge |
…imeSeries (#293) ## Summary [Recent changes](01a2f70#diff-deee840c61f04e77fc1a50be94e1e153c193fb4b00a7b1389a733e0941c29714) to `collectDoubles` in PivotUtils now return null instead of empty lists for invalid data. This broke the /timeseries endpoints which assumed non-null lists. Updated TimeSeriesHandler to: - Safely handle null lists using Option - Return empty sequences instead of failing - Fix isNumeric checks to handle null percentileDriftSeries @sean-zlai to test: You can check that the charts are broken on main, due to /timeseries being broken. If you check out this branch and run everything from scratch (`./docker-init/build.sh --all`), it should fix the charts. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved null handling in time series processing - Enhanced robustness of data conversion methods - Reduced risk of potential runtime exceptions <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Recent changes to
collectDoublesin PivotUtils now return null instead of empty lists for invalid data. This broke the /timeseries endpoints which assumed non-null lists.Updated TimeSeriesHandler to:
@sean-zlai to test:
You can check that the charts are broken on main, due to /timeseries being broken. If you check out this branch and run everything from scratch (
./docker-init/build.sh --all), it should fix the charts.Checklist
Summary by CodeRabbit