Use cached features in /rows #1573
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1573 +/- ##
==========================================
- Coverage 92.17% 90.44% -1.73%
==========================================
Files 77 193 +116
Lines 5443 12027 +6584
==========================================
+ Hits 5017 10878 +5861
- Misses 426 1149 +723
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
lhoestq
commented
Jul 27, 2023
severo
approved these changes
Jul 27, 2023
| parquet_files=response["content"]["parquet_files"], partial=response["content"]["partial"] | ||
| parquet_files=response["content"]["parquet_files"], | ||
| partial=response["content"]["partial"], | ||
| features=None, |
There was a problem hiding this comment.
what is it used for? better not to provide the field if it's always None
There was a problem hiding this comment.
config_parquet_content is instantiated for mypy typing I believe, but we don't return the features anyway in DatasetParquetResponse so no need to specify it.
I'll add a comment
There was a problem hiding this comment.
maybe we could test the other branch of the if/else
| @@ -30,6 +31,18 @@ class FileSystemError(Exception): | |||
| pass | |||
|
|
|||
|
|
|||
| def _clean_mongo_objects(obj: Any) -> Any: | |||
There was a problem hiding this comment.
maybe move this to the root function in simple_cache.py that gets the content out of the MongoDB?
|
also: I think we have to update the openapi spec |
ArgoCD Diff for commit
|
| Legend | Status |
|---|---|
| ✅ | The app is synced in ArgoCD, and diffs you see are solely from this PR. |
| The app is out-of-sync in ArgoCD, and the diffs you see include those changes plus any from this PR. | |
| 🛑 | There was an error generating the ArgoCD diffs due to changes in this PR. |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
/rows needs the cached
featuressince they're not always available in the parquet metadata.This was causing some
Imagecolumns to be seen as a struct of binary data, which are not supported in the viewer (shown as "null").Therefore I'm now passing the
featuresfromconfig-parquet-and-infotoconfig-parquetand then toconfig-parquet-metadata. I kept it backward compatible in case a cached value doesn't have this field yet.Therefore there's no need for a mongo migration. We can just re-run all the
config-parquetandconfig-parquet-metadatajobs. I incremented their versions.close #1421