You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2021/03/10 03:58:26 UTC
[GitHub] [datasketches-cpp] AlexanderSaydakov opened a new pull request #204: Unify serialization
AlexanderSaydakov opened a new pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204
Simplified and unified serialization and deserialization except HLL, where it is quite different form everything else, and will take a separate effort.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a change in pull request #204: Unify serialization
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204#discussion_r591797882
##########
File path: kll/include/kll_sketch_impl.hpp
##########
@@ -388,28 +388,28 @@ template<typename T, typename C, typename S, typename A>
void kll_sketch<T, C, S, A>::serialize(std::ostream& os) const {
const bool is_single_item = n_ == 1;
const uint8_t preamble_ints(is_empty() || is_single_item ? PREAMBLE_INTS_SHORT : PREAMBLE_INTS_FULL);
- os.write(reinterpret_cast<const char*>(&preamble_ints), sizeof(preamble_ints));
+ write(os, preamble_ints);
const uint8_t serial_version(is_single_item ? SERIAL_VERSION_2 : SERIAL_VERSION_1);
- os.write(reinterpret_cast<const char*>(&serial_version), sizeof(serial_version));
+ write(os, serial_version);
const uint8_t family(FAMILY);
- os.write(reinterpret_cast<const char*>(&family), sizeof(family));
+ write(os, family);
const uint8_t flags_byte(
(is_empty() ? 1 << flags::IS_EMPTY : 0)
| (is_level_zero_sorted_ ? 1 << flags::IS_LEVEL_ZERO_SORTED : 0)
| (is_single_item ? 1 << flags::IS_SINGLE_ITEM : 0)
);
- os.write(reinterpret_cast<const char*>(&flags_byte), sizeof(flags_byte));
- os.write((char*)&k_, sizeof(k_));
- os.write((char*)&m_, sizeof(m_));
+ write(os, flags_byte);
+ write(os, k_);
+ write(os, m_);
const uint8_t unused = 0;
- os.write(reinterpret_cast<const char*>(&unused), sizeof(unused));
+ write(os, unused);
if (is_empty()) return;
if (!is_single_item) {
- os.write((char*)&n_, sizeof(n_));
- os.write((char*)&min_k_, sizeof(min_k_));
- os.write((char*)&num_levels_, sizeof(num_levels_));
- os.write((char*)&unused, sizeof(unused));
- os.write((char*)levels_.data(), sizeof(levels_[0]) * num_levels_);
+ write(os, n_);
+ write(os, min_k_);
+ write(os, num_levels_);
+ write(os, unused);
+ os.write(reinterpret_cast<const char*>(levels_.data()), sizeof(levels_[0]) * num_levels_);
Review comment:
this is a different case. maybe we can have another template for this as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov merged pull request #204: Unify serialization
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov merged pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on a change in pull request #204: Unify serialization
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204#discussion_r591790485
##########
File path: kll/include/kll_sketch_impl.hpp
##########
@@ -388,28 +388,28 @@ template<typename T, typename C, typename S, typename A>
void kll_sketch<T, C, S, A>::serialize(std::ostream& os) const {
const bool is_single_item = n_ == 1;
const uint8_t preamble_ints(is_empty() || is_single_item ? PREAMBLE_INTS_SHORT : PREAMBLE_INTS_FULL);
- os.write(reinterpret_cast<const char*>(&preamble_ints), sizeof(preamble_ints));
+ write(os, preamble_ints);
const uint8_t serial_version(is_single_item ? SERIAL_VERSION_2 : SERIAL_VERSION_1);
- os.write(reinterpret_cast<const char*>(&serial_version), sizeof(serial_version));
+ write(os, serial_version);
const uint8_t family(FAMILY);
- os.write(reinterpret_cast<const char*>(&family), sizeof(family));
+ write(os, family);
const uint8_t flags_byte(
(is_empty() ? 1 << flags::IS_EMPTY : 0)
| (is_level_zero_sorted_ ? 1 << flags::IS_LEVEL_ZERO_SORTED : 0)
| (is_single_item ? 1 << flags::IS_SINGLE_ITEM : 0)
);
- os.write(reinterpret_cast<const char*>(&flags_byte), sizeof(flags_byte));
- os.write((char*)&k_, sizeof(k_));
- os.write((char*)&m_, sizeof(m_));
+ write(os, flags_byte);
+ write(os, k_);
+ write(os, m_);
const uint8_t unused = 0;
- os.write(reinterpret_cast<const char*>(&unused), sizeof(unused));
+ write(os, unused);
if (is_empty()) return;
if (!is_single_item) {
- os.write((char*)&n_, sizeof(n_));
- os.write((char*)&min_k_, sizeof(min_k_));
- os.write((char*)&num_levels_, sizeof(num_levels_));
- os.write((char*)&unused, sizeof(unused));
- os.write((char*)levels_.data(), sizeof(levels_[0]) * num_levels_);
+ write(os, n_);
+ write(os, min_k_);
+ write(os, num_levels_);
+ write(os, unused);
+ os.write(reinterpret_cast<const char*>(levels_.data()), sizeof(levels_[0]) * num_levels_);
Review comment:
Feels weird to have an os.write in the middle of this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] coveralls commented on pull request #204: Unify serialization
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204#issuecomment-796339483
## Pull Request Test Coverage Report for [Build 641188913](https://coveralls.io/builds/37835790)
* **42** of **42** **(100.0%)** changed or added relevant lines in **1** file are covered.
* **3** unchanged lines in **1** file lost coverage.
* Overall coverage decreased (**-0.2%**) to **93.156%**
---
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [theta/include/bounds_on_ratios_in_theta_sketched_sets.hpp](https://coveralls.io/builds/37835790/source?filename=theta%2Finclude%2Fbounds_on_ratios_in_theta_sketched_sets.hpp#L66) | 3 | 80.0% |
<!-- | **Total:** | **3** | | -->
| Totals | [](https://coveralls.io/builds/37835790) |
| :-- | --: |
| Change from base [Build 612226124](https://coveralls.io/builds/37556720): | -0.2% |
| Covered Lines: | 2246 |
| Relevant Lines: | 2411 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] coveralls edited a comment on pull request #204: Unify serialization
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204#issuecomment-796339483
## Pull Request Test Coverage Report for [Build 644969319](https://coveralls.io/builds/37872147)
* **45** of **45** **(100.0%)** changed or added relevant lines in **1** file are covered.
* **3** unchanged lines in **1** file lost coverage.
* Overall coverage decreased (**-0.2%**) to **93.156%**
---
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [theta/include/bounds_on_ratios_in_theta_sketched_sets.hpp](https://coveralls.io/builds/37872147/source?filename=theta%2Finclude%2Fbounds_on_ratios_in_theta_sketched_sets.hpp#L66) | 3 | 80.0% |
<!-- | **Total:** | **3** | | -->
| Totals | [](https://coveralls.io/builds/37872147) |
| :-- | --: |
| Change from base [Build 612226124](https://coveralls.io/builds/37556720): | -0.2% |
| Covered Lines: | 2246 |
| Relevant Lines: | 2411 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #204: Unify serialization
Posted by GitBox <gi...@apache.org>.
jmalkin commented on pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204#issuecomment-794893185
I think we need to figure out what happened to code coverage.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] coveralls edited a comment on pull request #204: Unify serialization
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #204:
URL: https://github.com/apache/datasketches-cpp/pull/204#issuecomment-796339483
## Pull Request Test Coverage Report for [Build 647579939](https://coveralls.io/builds/37897497)
* **73** of **73** **(100.0%)** changed or added relevant lines in **3** files are covered.
* **3** unchanged lines in **1** file lost coverage.
* Overall coverage decreased (**-0.2%**) to **93.156%**
---
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [theta/include/bounds_on_ratios_in_theta_sketched_sets.hpp](https://coveralls.io/builds/37897497/source?filename=theta%2Finclude%2Fbounds_on_ratios_in_theta_sketched_sets.hpp#L66) | 3 | 80.0% |
<!-- | **Total:** | **3** | | -->
| Totals | [](https://coveralls.io/builds/37897497) |
| :-- | --: |
| Change from base [Build 612226124](https://coveralls.io/builds/37556720): | -0.2% |
| Covered Lines: | 2246 |
| Relevant Lines: | 2411 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org