Add support for getting freshness from DBMS metadata #8795
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8795 +/- ##
==========================================
- Coverage 86.44% 83.30% -3.15%
==========================================
Files 176 177 +1
Lines 26046 26130 +84
==========================================
- Hits 22516 21767 -749
- Misses 3530 4363 +833
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| adapter_response, table = result.response, result.table # type: ignore[attr-defined] | ||
|
|
||
| try: | ||
| row = table[0] |
There was a problem hiding this comment.
Can you just load the first row as a dict then do a simple key lookup?
Might make it slightly easier to debug.
There was a problem hiding this comment.
This looks good. The only issue I saw was that we're still pulling Capability stuff from base/impl.py in some spots and we should update that to pull from the new capability module. I noted it wherever I saw that. But aside from that (and the protobuf merge conflicts), I'm comfortable with this. I'll give it an approval from the Adapters side with the caveat of those import updates.
| @@ -1203,7 +1213,7 @@ def calculate_freshness( | |||
| loaded_at_field: str, | |||
| filter: Optional[str], | |||
| manifest: Optional[Manifest] = None, | |||
| ) -> Tuple[Optional[AdapterResponse], Dict[str, Any]]: | |||
| ) -> Tuple[Optional[AdapterResponse], FreshnessResponse]: | |||
There was a problem hiding this comment.
I think this will cause mypy errors if it's overridden, but I don't think it breaks the functional code since the two types are almost the same; it should only break linting. The exception is if a maintainer is passing freshness information as a str or other data type in the value of the original dict, which is already problematic. Worth pointing out to @anders for the 1.7 upgrade guide for adapter maintainers.
resolves #8704
Problem
Solution
Checklist