Autogenerate aggregation responses and hits #1932
Conversation
miguelgrinberg
force-pushed
the
autogenerate-aggregations-and-hit
branch
4 times, most recently
from
b6f3fe4 to
b218090
Compare
miguelgrinberg
force-pushed
the
autogenerate-aggregations-and-hit
branch
from
b218090 to
9777a93
Compare
|
|
||
|
|
||
| class HitMeta(HitMetaBase): | ||
| inner_hits: Mapping[str, Any] |
There was a problem hiding this comment.
I haven't found a way to type inner hints appropriately without making breaking changes. I think if we have the intention to have proper typing we'll have to evaluate a change here.
For example, consider:
hit.meta.inner_hits["passages"][0].contentHere it is impossible to type the content attribute of the first Passage document, since inner_hits is a dictionary, and each entry in this dict maps to a different document type.
We may consider changing to something like:
hit.passages.meta.inner_hits[0].contentI think this may be possible to type because the inner hits belong to the nested document, and hit.passages already has the correct type. The problem is that hit.passages is a list, so adding attributes to the list feels weird.
In any case, this feels like a bigger change that cannot be rushed.
| @@ -30,7 +31,7 @@ async def scan_aggs( | |||
| source_aggs: Sequence[Mapping[str, Agg]], | |||
| inner_aggs: Dict[str, Agg] = {}, | |||
| size: int = 10, | |||
| ) -> AsyncIterator[Response]: | |||
There was a problem hiding this comment.
Response here was plainly wrong, not sure how this passed mypy to be honest!
| @@ -161,8 +159,6 @@ async def add_answer( | |||
| answer = Answer( | |||
| # required make sure the answer is stored in the same shard | |||
| _routing=self.meta.id, | |||
| # since we don't have explicit index, ensure same index as self | |||
| _index=self.meta.index, | |||
There was a problem hiding this comment.
This wasn't necessary on this example, since all the documents belong to the same index. I removed it because it creates a typing problem, because _index is stored as an Index or AsyncIndex, but here it was given as a string.
| @@ -625,6 +638,9 @@ def interface_to_python_class( | |||
| "required": bool, | |||
| "positional": bool, | |||
| ], | |||
| "buckets_as_dict": "type" # optional, only present in aggregation response | |||
| # classes that have buckets that can have a list | |||
| # or dict representation | |||
There was a problem hiding this comment.
To make typing easier aggregations have buckets typed as a list and buckets_as_dict typed as a dict. You can use whichever makes sense depending on how yo request your aggregations. It's a compromise solution to avoid having to apply casts on a type that is a union of list and dict buckets.
There was a problem hiding this comment.
What do you mean with "type" here?
There was a problem hiding this comment.
"type" here is the type that the buckets_as_dict property will be. This is used on the types Jinja template:
@property
def buckets_as_dict(self) -> Mapping[str, {{ k.buckets_as_dict }}]:
return self.buckets # type: ignore
| yield b | ||
| if "after_key" in response.aggregations.comp: | ||
| after = response.aggregations.comp.after_key | ||
| while response.aggregations["comp"].buckets: |
There was a problem hiding this comment.
Switching from .comp to ["comp"] feels like a net negative, but if that helps mypy type correctly (and raise an error on a typo) then that's OK!
There was a problem hiding this comment.
I'm not sure I agree. To me it feels like using the dict syntax makes it clear that this is a dynamic data item that is application specific. But maybe I convinced myself this is better.
* typing of aggregation responses * buckets_as_dict property * typing of the `meta` attribute of hits * more minor adjustments to response types (cherry picked from commit 601bea7)
* typing of aggregation responses * buckets_as_dict property * typing of the `meta` attribute of hits * more minor adjustments to response types (cherry picked from commit 601bea7) Co-authored-by: Miguel Grinberg <[email protected]>
* typing of aggregation responses * buckets_as_dict property * typing of the `meta` attribute of hits * more minor adjustments to response types
No description provided.