Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set separate coordinate buffers to write queries #1458

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

stavrospapadopoulos
Copy link
Member

Towards addressing #93

Copy link
Member

@Shelnutt2 Shelnutt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serialization changes look good.

Copy link
Contributor

@joe-maley joe-maley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to worry about serialization compatibility with the REST server? For instance, if the REST server returns a query serialized with an older version, would we be able to properly deserialize it?

@@ -291,6 +296,18 @@ const Dimension* ArraySchema::dimension(unsigned int i) const {
return domain_->dimension(i);
}

const Dimension* ArraySchema::dimension(std::string name) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const-ref name

std::vector<std::string> ret;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const size_t ret_size = attr_buffers_.size() + (coords_buffer_ ? 1 : coord_buffers_.size())
ret.reserve(ret_size)

@Shelnutt2
Copy link
Member

Do we need to worry about serialization compatibility with the REST server? For instance, if the REST server returns a query serialized with an older version, would we be able to properly deserialize it?

The serialization format is buffer agnostic, previously we were using the list of attributes + coords these changes just rework things so instead of loop over attributes it loops over the list of buffers. For serialization it loops over the query buffer list, for deserialization we loop over the buffers as listed in the copy_state which comes from the attributeBufferHeaders object in capnproto spec. attributeBufferHeaders is an incorrect name, it really should be bufferHeaders but changing the name would break compatibility so it is left.

@stavrospapadopoulos
Copy link
Member Author

@joe-maley the serialization format did not change in this PR. At a high level, the query serializes a set of named buffers. Supporting separate coordinate buffers really means allowing the named buffers to carry dimension names in addition to attributes and TILEDB_COORDS. This PR lifts some checks that mandated that the named buffers always correspond to attributes and TILEDB_COORDS.

@joe-maley
Copy link
Contributor

Thanks! Just checking. LGTM.

@stavrospapadopoulos stavrospapadopoulos force-pushed the sp/writer_split_coords branch 2 times, most recently from 53e5bef to 9b0df7e Compare December 13, 2019 15:22
@stavrospapadopoulos stavrospapadopoulos merged commit a6c5381 into dev Dec 13, 2019
@stavrospapadopoulos stavrospapadopoulos deleted the sp/writer_split_coords branch December 13, 2019 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants