Segments improvements: static RPM detector, dashboard queries, batch perf#249
Segments improvements: static RPM detector, dashboard queries, batch perf#249
Conversation
zer0stars
force-pushed
the
segments-improvements
branch
4 times, most recently
from
f145702 to
91ce65f
Compare
|
Still reviewing this, but one comment I'd like to make is that when reviewing it's nice for me, at least, having not too many memory cells, to have focused PRs. The unrelated changes to events, attestations, etc., make it harder to get at the heart of the matter. I'm going to try to just ignore the "other" stuff and focus on segments. |
elffjs
left a comment
elffjs
left a comment
There was a problem hiding this comment.
I think we should merge. Whatever quibbles I have about the .graphql have already been set in stone. The implementation seems fine, I need more time to digest it, but it seems fine.
| eventDataSummary: [eventDataSummary!]! | ||
| } | ||
|
|
||
| type eventDataSummary { |
There was a problem hiding this comment.
I don't understand why all the types are lowercased. I don't think I've ever seen that. See, e.g., the official site.
There was a problem hiding this comment.
I guess Kevin had this pattern here before? This is truly bizarre.
There was a problem hiding this comment.
Just following his convention, let's update after merge.
| Result of aggregating a float signal over an interval. Used by segments and daily activity summaries. | ||
| Same shape as one row of aggregated signal data (name, aggregation type, computed value). | ||
| """ | ||
| type SignalAggregationValue { |
There was a problem hiding this comment.
I think this speaks to how the segment stuff has a fundamentally different structure. Are we okay with this? It's sort of the difference between wide and thin tables. Originally our schema was
windowedQuery(start: "..", end: "..", step: "..") {
windowTimestamp
speed(agg: MAX): Float
..
}
and now it's, I think,
windowedQuery(start: "..", end: "..", step: "..", signals: []) {
windowTimestamp
aggregations {
signalName
aggregation
value
}
}
Very different. It's nice to be consistent but it's not the end of the world, but I guess I'd like to see some reasoning.
There was a problem hiding this comment.
I thought about this alot. The other option would have been extending the object to: segments(...) { start { timestamp location { latitude longitude } signals { fuelLevel(agg: FIRST) odometer(agg: FIRST) } } end { timestamp location { latitude longitude } signals { fuelLevel(agg: LAST) odometer(agg: LAST) } } signals { speed(agg: MAX) } }
Which was arguably worse. You could make the argument thats a normal api call to make to get max speed over a segment. You would probably send more queries regardless.
There was a problem hiding this comment.
I mean, I could argue either way. At this moment, I don’t see a strong argument for being inconsistent.
Anyway, it seems like this is what we’re going to do, so forget I said anything
| """ | ||
| Idling: Segments are contiguous periods where engine RPM remains in idle range. | ||
| """ | ||
| idling |
There was a problem hiding this comment.
Why are these enums lowercased? This doesn't seem typical and we didn't do it for the aggregations.
| Segment IDs are stable and consistent across queries as long as the segment start | ||
| is captured in the underlying data source. | ||
|
|
||
| Each segment includes summary: signals, start/end location, and (when requested) eventCounts. |
There was a problem hiding this comment.
Missing a word, maybe?. "a summary"?
…l/event summaries - New detectors: RechargeDetector (SoC trough-to-peak), RefuelDetector (fuel rise windows), IdlingDetector (RPM-based), plus improvements to FrequencyDetector, ChangePointDetector, and IgnitionDetector. - Daily activity query with per-day segment + event summaries. - Batch aggregation queries (signals and events) for segment summaries with start/end SignalLocation. - Shared utilities: mergeTimeRanges, clipTimeRange, getLevelSamples, getWindowedSignalCounts with unified merge pipeline. - GraphQL schema extensions for new mechanisms, configs, and response types. - E2E and unit tests for all new detectors. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…minDuration - clipTimeRange: allow segments that started before 'from' if original duration meets minDuration (fixes FrequencyDetector and ChangePointDetector StartedBeforeRange e2e tests) - .gitignore: remove stale entry for deleted e2e/local_data_test.go
zer0stars
force-pushed
the
segments-improvements
branch
from
3af66a4 to
22d0a66
Compare
2 participants
Summary
Single squashed commit with all segments-improvements work.
Features
mechanism: staticRpmfor contiguous idle-RPM segments; configurablemaxIdleRpm,minSegmentDurationSeconds.dailyActivityquery: one record per calendar day (segment count, total active time, locations, optional signal/event summaries). Max 30 days;ignitionDetection,frequencyAnalysis, orchangePointDetectiononly (no staticRpm).segmentswithsignalSummaries/eventSummariesfor per-segment aggregates and event counts; pagination vialimit/after; default signal set when summaries requested but not specified.LoadSampleDataIntoinclickhouse_container_test.gofor loading sample_data CSVs (used bylocal_data_test); errcheck-safedeferfor file closes.charts/aligned with main.batchFloatCaseExpr/batchLocationCaseExprininternal/service/ch/queries.go; errcheck fixes forClose().