feat(records): aggregate endpoint#2705
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eneric base tests Aggregate request builders raise NotImplementedError in _load and result classes have non-standard constructors, so they cannot participate in the generic dump/load round-trip tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2705 +/- ##
==========================================
+ Coverage 93.71% 93.72% +0.01%
==========================================
Files 504 504
Lines 51294 51680 +386
==========================================
+ Hits 48070 48439 +369
- Misses 3224 3241 +17
🚀 New features to boost your workflow:
|
…aggregate classes from base tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…perty Encapsulates the parsed bucket list behind a private attribute so callers can't mutate cached state, and drops the redundant one-line docstrings that only restated the class names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces record aggregation capabilities to the data modeling records API, adding async and sync aggregate methods alongside various typed aggregate request builders and result classes. The review feedback suggests refactoring RecordsAggregate to not inherit from CogniteResource since it functions as a request builder rather than a DTO, which will also simplify unit test exclusions. Additionally, it is recommended to use a more specific type hint for the aggregates parameter to avoid the use of Any.
| >>> res = client.data_modeling.records.aggregate( | ||
| ... stream_id="my-stream", | ||
| ... aggregates={ | ||
| ... "avg_temp": Avg(["my-space", "sensor", "temperature"]), |
There was a problem hiding this comment.
Let's make it more clear that this is a property reference.
Ref docs:
the first segment is the space that the container belongs to,
the second segment is the container external id,
the third segment is the property id inside the container.
There was a problem hiding this comment.
specified the property key in the examples
There was a problem hiding this comment.
I think we should keep these out of cognite/client/data_classes/data_modeling/__init__.py and only in ...records.py
There was a problem hiding this comment.
just a heads up it will be inconsistent with the other data classes that are accessible there. Do you want purely aggregate related classes, or all records/streams classes to be in their own module?
There was a problem hiding this comment.
I've prefixed the response agg types with records, and then i moved the builder types into the records module. Do you want me to move the response types into records as well?
|
|
||
| Examples: | ||
|
|
||
| Aggregate average temperature using typed helpers: |
There was a problem hiding this comment.
I also wonder a bit about these examples - they feel like examples of what kind of data that belongs in the Datapoints service, not the Records service. Could you ask Claude to come up with different examples? 😅
There was a problem hiding this comment.
re-used the examples from the cognite api docs
| @classmethod | ||
| def _load(cls, resource: dict[str, Any]) -> Self: | ||
| raise NotImplementedError(f"{cls.__name__} is a request builder and cannot be loaded from API responses") |
There was a problem hiding this comment.
In v7 of the SDK, load was made public so that all request and response classes could be dumped (and loaded back) into config files, thus the _load method here need to be supported.
| for result_cls in ( | ||
| UniqueValuesAggregateResult, | ||
| NumberHistogramAggregateResult, | ||
| TimeHistogramAggregateResult, | ||
| FilterAggregateResult, | ||
| ): | ||
| if result_cls._buckets_key in resource: | ||
| return result_cls._load(resource) |
There was a problem hiding this comment.
Same comment here, I would rather have you build a mapping as a module-level constant for constant-time lookup
| def __init__(self, raw_result: dict[str, Any]) -> None: | ||
| self._raw_result = raw_result | ||
|
|
||
| def dump(self, camel_case: bool = True) -> dict[str, Any]: | ||
| return self._raw_result |
There was a problem hiding this comment.
This seem a bit lazy tbh 😆 This camel_case param is being ignored
| def __init__(self, raw_bucket: dict[str, Any]) -> None: | ||
| self._raw_bucket = raw_bucket | ||
| self.count = raw_bucket["count"] | ||
| self.value = raw_bucket.get("value") | ||
| self.interval_start = raw_bucket.get("intervalStart") | ||
| self.aggregates = raw_bucket.get("aggregates", {}) | ||
| self.results = { | ||
| aggregate_id: RecordsAggregateResult._load(result) | ||
| for aggregate_id, result in self.aggregates.items() | ||
| if isinstance(result, dict) | ||
| } | ||
|
|
||
| @classmethod | ||
| def _load(cls, resource: dict[str, Any]) -> Self: | ||
| return cls(resource) |
There was a problem hiding this comment.
This init looks like load
|
|
||
| Args: | ||
| aggregates (dict[str, Any]): Aggregate results keyed by the client-defined aggregate IDs. | ||
| typing (TypeInformation | None): Optional property typing metadata. |
There was a problem hiding this comment.
Feels weird to me to have this typing info randomly put here, but I guess you are just mimicking DMS? Should we consider dropping that from /aggregate until someone requests it?
| }, | ||
| } | ||
|
|
||
| def test_records_aggregation_dump_round_trip(self) -> None: |
There was a problem hiding this comment.
Tested automatically, unless you opted out
Co-authored-by: Håkon V. Treider <haakonvt@gmail.com>
move out of dms module, switch to dump only, and add a bunch more tests
| return {"property": self.property} | ||
|
|
||
|
|
||
| class Avg(_PropertyAggregate): |
There was a problem hiding this comment.
Any reason we can't reuse the existing classes in data_classes/aggregations?
There was a problem hiding this comment.
Those seem to be designed around views, and have different logic for refering to a property. We also have aggregates that aren't handled there++. So to me it got a bit messy, and I went with our own. Though I hope we can consolidate over time.
There was a problem hiding this comment.
Those seem to be designed around views
There's no mention of views there, so not sure what you're referring to.
have different logic for refering to a property.
I thought we were referencing properties in the same way everywhere? What's the deviation?
We also have aggregates that aren't handled there
You can add new aggregates to that module

Description
Introduces support for the aggregate endpoint for the Records collection.