Allow updating multiple view representations#15
Allow updating multiple view representations#15SergeiPatiakin wants to merge 19 commits intomainfrom
Conversation
To support compressed metadata files, we need to decompress them before converting the data into json.
feat: align GCS scheme name with other libs
SergeiPatiakin
force-pushed
the
upsert-representation-2
branch
from
805e3a3 to
7b373db
Compare
SergeiPatiakin
changed the title
| version.default_namespace = namespace.to_vec() | ||
| } | ||
| if version.default_catalog().is_none() { | ||
| if version.default_catalog().is_none() && !catalog.name().is_empty() { |
There was a problem hiding this comment.
This code (whole if let some(version)) is repeating with the one on line 220 and could be moved to a separate function.
| for r in new_representations { | ||
| representations = upsert_representation(&representations, r.clone()); | ||
| } |
There was a problem hiding this comment.
I really dislike such naming. For such a small code block is easy to see what it is, but if for-block grows, it becomes annoying to figure it out what the single letter variable is.
The mut avoids cloning and rebuilding representations for each new representation.
| for r in new_representations { | |
| representations = upsert_representation(&representations, r.clone()); | |
| } | |
| for representation in new_representations { | |
| representations = upsert_representation(&mut representations, representation.clone()); | |
| } |
| fn upsert_representation( | ||
| current_representations: &[ViewRepresentation], | ||
| new_representation: ViewRepresentation, | ||
| ) -> Vec<ViewRepresentation> { |
There was a problem hiding this comment.
This function allocates a new Vec every time it’s called, and you call it in a loop on line 73. If new_representations contains N elements and current_representations contains M, this results in O(N * M) copies and allocations.
| fn upsert_representation( | |
| current_representations: &[ViewRepresentation], | |
| new_representation: ViewRepresentation, | |
| ) -> Vec<ViewRepresentation> { | |
| fn upsert_representation( | |
| representations: &mut Vec<ViewRepresentation>, | |
| new_representation: ViewRepresentation, | |
| ) -> Vec<ViewRepresentation> { |
There was a problem hiding this comment.
True, but I don't expect the number of representations to ever exceed single digits. There's not that many different analytics systems with incompatible representations.
|
Closing because merged upstream: JanKaul#181 |
5 participants
Operation::UpdateRepresentationtoOperation::UpdateRepresentationswhich can take multiple representationsOperation::UpdateRepresentationspreserves representations in other dialects instead of overwriting them