-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: add aggregateGroupByPeriod API #157
base: main
Are you sure you want to change the base?
Conversation
Hey! Thanks for the PR :) I will try to review it this weekend if I find some free time. |
@matinzd Do you have time to take a look at the PR by any chance? Once this is approved, I can make another PR very similar to this for aggregateGroupByDuration. |
Hey! I reviewed 60% of it. There are a lot of file changes so I need to be careful with the review :) Thanks for the follow-up. |
fun mapPeriodStringToPeriod(period: String?): Period { | ||
return when (period) { | ||
"DAYS" -> Period.ofDays(1) | ||
"WEEKS" -> Period.ofWeeks(1) | ||
"MONTHS" -> Period.ofMonths(1) | ||
"YEARS" -> Period.ofYears(1) | ||
else -> Period.ofDays(1) | ||
} | ||
} |
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.
I would rather to keep the data types dynamic as much as possible. Maybe we could accept ReadableMap
that contains both slice period and the number instead of defaulting all of them to 1.
fun mapJsPeriodToPeriod(period: ReadableMap?): Period {
return when (period.getString("period")) {
"DAYS" -> Period.ofDays(period.getInt("duration"))
"WEEKS" -> Period.ofWeeks(period.getInt("duration")
"MONTHS" -> Period.ofMonths(period.getInt("duration")
"YEARS" -> Period.ofYears(period.getInt("duration")
else -> Period.ofDays(period.getInt("duration")
}
}
src/types/base.types.ts
Outdated
@@ -33,6 +33,8 @@ export type TimeRangeFilter = | |||
endTime: string; | |||
}; | |||
|
|||
export type TimeRangeSlicer = 'DAYS' | 'WEEKS' | 'MONTHS' | 'YEARS'; |
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.
Then the type here need to be adjusted.
interface TimeRangeSlicer {
period: 'DAYS' | 'WEEKS' | 'MONTHS' | 'YEARS'
duration: number
}
@matinzd Thank you for the review! I will apply the changes soon |
@matinzd I have applied the changes accordingly. And here is the log message for the record. Log after AGGREGATE SAMPLE GROUP DATA button is pressed# The user timezone is Asia/Tokyo
# No sample data is inserted for TODAY (2024-10-02)
Aggregated Group: {
"result": [
{
"endTime": "2024-09-25T15:00",
"startTime": "2024-09-24T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 1000
}
},
{
"endTime": "2024-09-26T15:00",
"startTime": "2024-09-25T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 2000
}
},
{
"endTime": "2024-09-27T15:00",
"startTime": "2024-09-26T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 3000
}
},
{
"endTime": "2024-09-28T15:00",
"startTime": "2024-09-27T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 4000
}
},
{
"endTime": "2024-09-29T15:00",
"startTime": "2024-09-28T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 5000
}
},
{
"endTime": "2024-09-30T15:00",
"startTime": "2024-09-29T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 6000
}
},
{
"endTime": "2024-10-01T15:00",
"startTime": "2024-09-30T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 7000
}
},
{
"endTime": "2024-10-02T12:47:21.255",
"startTime": "2024-10-01T15:00",
"result": {
"dataOrigins": [],
"COUNT_TOTAL": 0
}
}
]
} |
Summary
Currently, the library only supports the
aggregateRecord
function to retrieve aggregated data within a specified time range and does not support eitheraggregateGroupByPeriod
oraggregateGroupByDuration
. This PR adds the aggregateGroupByPeriod API to the library. Once this is approved, I am ready to submit another PR for aggregateGroupByDuration and update the documentation accordingly.Related Issues:
#16
#116
Background
I'm working on an app that visualizes various health data in charts. For example, to display daily weight changes over a month, we currently need to make 30 separate
aggregateRecord
requests, one for each day. WithaggregateGroupByPeriod
, we can potentially reduce the number of requests significantly. I suspect that the following error, which I occasionally encounter, may be related to exceeding the API call quota:Tests
I've made a couple of changes to the example app to test the new functionality:
insertSampleData
function now inserts step count data for each day between 9 and 11 AM for the past 7 days (excluding today, as it can be before 9 AM).aggregateGroupByPeriod
API.Log after
AGGREGATE SAMPLE GROUP DATA
button is pressed