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

CapnProtoInputStream jagged structures #4063

Merged
merged 15 commits into from
Jan 17, 2019
Merged

CapnProtoInputStream jagged structures #4063

merged 15 commits into from
Jan 17, 2019

Conversation

Miniwoffer
Copy link

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog. Remove if this is non-significant change.

Category:

  • Improvement

Short description:
Changed the way CapnProtoInputStream creates actions in such a way that it now support structures that are jagged.

Detailed description:
CapnProtoInputStream only worked with CapnProto objects where at most one child could have children of its own.

To fix backtracking was implemented.

Also, added '' as separator in addition to '.', this should not cause problems since '' is not allowed in CapnProto names.

Changed how actions are created for CapnProto so it supports jagged
structures.
Added _ as delim
This makes it so you do not have to use . when addresing structures
and since Capnproto can not use _ in its naming its fine to use.
 - Added some comments.
 - Immplemented a recurisve parent check so fields with parents by
the same name but grandparents with diffrent names dont cause problems
@alexey-milovidov
Copy link
Member

Missing test.

@Miniwoffer
Copy link
Author

Okay, not sure what you want me to use instead of the "checkEqualFrom" function, please elaborate.

@alexey-milovidov
Copy link
Member

I have some troubles reading this code and I've removed the function checkEqualFrom by mistake.

alexey-milovidov added a commit that referenced this pull request Jan 17, 2019
@alexey-milovidov
Copy link
Member

I've fixed build but your test is not passed:

Field nestedone_nestednestedone_nestednestednumber doesn't exist in schema CapnProto

alexey-milovidov added a commit that referenced this pull request Jan 17, 2019
@alexey-milovidov
Copy link
Member

How did you generated Cap'n'Proto test message?

@Miniwoffer
Copy link
Author

With the cli tool
capnp convert json:binary test.capnp CapnProto < test.json
I can supply the json file if needed

alexey-milovidov added a commit that referenced this pull request Jan 17, 2019
@alexey-milovidov alexey-milovidov merged commit af32828 into ClickHouse:master Jan 17, 2019
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Jan 17, 2019

Ok.

The code is a bit difficult to understand for me. I have rewritten it a little and now the test passed.
But I'm not sure that my modifications are correct. Can you please review my modifications?

(and add more tests if needed in separate PR)

@Miniwoffer
Copy link
Author

I'm sorry this is a little late. It looks good, i also did some manual testing and wrote two additional tests that takes care of edge cases if there are changes in the future.
The tests are not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants