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/02/12 00:08:02 UTC
[GitHub] [datasketches-cpp] AlexanderSaydakov opened a new pull request #195: test that allocator default constructor is not called
AlexanderSaydakov opened a new pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195
----------------------------------------------------------------
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 #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#discussion_r575411408
##########
File path: sampling/test/var_opt_allocation_test.cpp
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <var_opt_sketch.hpp>
+#include <var_opt_union.hpp>
+#include <test_type.hpp>
+#include <test_allocator.hpp>
+
+#include <catch.hpp>
+
+#include <sstream>
+
+namespace datasketches {
+
+using var_opt_test_sketch = var_opt_sketch<test_type, test_type_serde, test_allocator<test_type>>;
+using var_opt_test_union = var_opt_union<test_type, test_type_serde, test_allocator<test_type>>;
+
+TEST_CASE("varopt allocation test", "[var_opt_sketch]") {
+ test_allocator_total_bytes = 0;
+ test_allocator_net_allocations = 0;
+ {
+ var_opt_test_sketch sk1(10, var_opt_test_sketch::DEFAULT_RESIZE_FACTOR, 0);
+ for (int i = 0; i < 100; ++i) sk1.update(i);
+ auto bytes1 = sk1.serialize();
+ auto sk2 = var_opt_test_sketch::deserialize(bytes1.data(), bytes1.size(), 0);
+
+ std::stringstream ss;
+ sk1.serialize(ss);
+ auto sk3 = var_opt_test_sketch::deserialize(ss, 0);
+
+ var_opt_test_union u1(10, 0);
+ u1.update(sk1);
+ u1.update(sk2);
+ u1.update(sk3);
+
+ auto bytes2 = u1.serialize();
+ auto u2 = var_opt_test_union::deserialize(bytes2.data(), bytes2.size(), 0);
+ }
+ REQUIRE(test_allocator_total_bytes == 0);
+ REQUIRE(test_allocator_net_allocations == 0);
+}
+
+TEST_CASE( "varopt union: move", "[var_opt_union][test_type]") {
+ uint32_t n = 20;
+ uint32_t k = 5;
+ var_opt_union<test_type> u(k);
+ var_opt_sketch<test_type> sk1(k);
+ var_opt_sketch<test_type> sk2(k);
+
+ // move udpates
+ for (int i = 0; i < (int) n; ++i) {
+ sk1.update(i);
+ sk2.update(-i);
+ }
+ REQUIRE(sk1.get_n() == n);
+ REQUIRE(sk2.get_n() == n);
+
+ // move unions
+ u.update(std::move(sk2));
+ u.update(std::move(sk1));
+ REQUIRE(u.get_result().get_n() == 2 * n);
+
+ // move constructor
+ var_opt_union<test_type> u2(std::move(u));
+ REQUIRE(u2.get_result().get_n() == 2 * n);
+
+ // move assignment
+ var_opt_union<test_type> u3(k);
+ u3 = std::move(u2);
+ REQUIRE(u3.get_result().get_n() == 2 * n);
Review comment:
Right, this particular test was moved as-is from var_opt_union_test.cpp because the test_type (as it currently is) can be included only in one file per module.
----------------------------------------------------------------
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 #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#issuecomment-777881282
## Pull Request Test Coverage Report for [Build 561866240](https://coveralls.io/builds/37096949)
* **0** of **0** changed or added relevant lines in **0** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.2%**) to **94.522%**
---
| Totals | [](https://coveralls.io/builds/37096949) |
| :-- | --: |
| Change from base [Build 556164680](https://coveralls.io/builds/37039799): | 0.2% |
| Covered Lines: | 4417 |
| Relevant Lines: | 4673 |
---
##### 💛 - [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 a change in pull request #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#discussion_r575115764
##########
File path: sampling/test/var_opt_allocation_test.cpp
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <var_opt_sketch.hpp>
+#include <var_opt_union.hpp>
+#include <test_type.hpp>
+#include <test_allocator.hpp>
+
+#include <catch.hpp>
+
+#include <sstream>
+
+namespace datasketches {
+
+using var_opt_test_sketch = var_opt_sketch<test_type, test_type_serde, test_allocator<test_type>>;
+using var_opt_test_union = var_opt_union<test_type, test_type_serde, test_allocator<test_type>>;
+
+TEST_CASE("varopt allocation test", "[var_opt_sketch]") {
+ test_allocator_total_bytes = 0;
+ test_allocator_net_allocations = 0;
+ {
+ var_opt_test_sketch sk1(10, var_opt_test_sketch::DEFAULT_RESIZE_FACTOR, 0);
+ for (int i = 0; i < 100; ++i) sk1.update(i);
+ auto bytes1 = sk1.serialize();
+ auto sk2 = var_opt_test_sketch::deserialize(bytes1.data(), bytes1.size(), 0);
+
+ std::stringstream ss;
+ sk1.serialize(ss);
+ auto sk3 = var_opt_test_sketch::deserialize(ss, 0);
+
+ var_opt_test_union u1(10, 0);
+ u1.update(sk1);
+ u1.update(sk2);
+ u1.update(sk3);
+
+ auto bytes2 = u1.serialize();
+ auto u2 = var_opt_test_union::deserialize(bytes2.data(), bytes2.size(), 0);
+ }
+ REQUIRE(test_allocator_total_bytes == 0);
+ REQUIRE(test_allocator_net_allocations == 0);
+}
+
+TEST_CASE( "varopt union: move", "[var_opt_union][test_type]") {
+ uint32_t n = 20;
+ uint32_t k = 5;
+ var_opt_union<test_type> u(k);
+ var_opt_sketch<test_type> sk1(k);
+ var_opt_sketch<test_type> sk2(k);
+
+ // move udpates
+ for (int i = 0; i < (int) n; ++i) {
+ sk1.update(i);
+ sk2.update(-i);
+ }
+ REQUIRE(sk1.get_n() == n);
+ REQUIRE(sk2.get_n() == n);
+
+ // move unions
+ u.update(std::move(sk2));
+ u.update(std::move(sk1));
+ REQUIRE(u.get_result().get_n() == 2 * n);
+
+ // move constructor
+ var_opt_union<test_type> u2(std::move(u));
+ REQUIRE(u2.get_result().get_n() == 2 * n);
+
+ // move assignment
+ var_opt_union<test_type> u3(k);
+ u3 = std::move(u2);
+ REQUIRE(u3.get_result().get_n() == 2 * n);
Review comment:
Do we want to check for total bytes allocated here?
##########
File path: hll/test/HllSketchTest.cpp
##########
@@ -162,27 +162,23 @@ TEST_CASE("hll sketch: check ser sizes", "[hll_sketch]") {
}
TEST_CASE("hll sketch: exercise to string", "[hll_sketch]") {
- test_allocator_total_bytes = 0;
- {
- hll_sketch_test_alloc sk(15, HLL_4);
- for (int i = 0; i < 25; ++i) { sk.update(i); }
- std::ostringstream oss(std::ios::binary);
- oss << sk.to_string(false, true, true, true);
- for (int i = 25; i < (1 << 20); ++i) { sk.update(i); }
- oss << sk.to_string(false, true, true, true);
- oss << sk.to_string(false, true, true, false);
-
- sk = hll_sketch_test_alloc(8, HLL_8);
- for (int i = 0; i < 25; ++i) { sk.update(i); }
- oss << sk.to_string(false, true, true, true);
- }
- REQUIRE(test_allocator_total_bytes == 0);
+ hll_sketch sk(15, HLL_4);
Review comment:
Seems reasonable to have some non-test_allocator usage, too, but why was it explicitly removed in this case?
----------------------------------------------------------------
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 #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#discussion_r575423373
##########
File path: hll/test/HllSketchTest.cpp
##########
@@ -162,27 +162,23 @@ TEST_CASE("hll sketch: check ser sizes", "[hll_sketch]") {
}
TEST_CASE("hll sketch: exercise to string", "[hll_sketch]") {
- test_allocator_total_bytes = 0;
- {
- hll_sketch_test_alloc sk(15, HLL_4);
- for (int i = 0; i < 25; ++i) { sk.update(i); }
- std::ostringstream oss(std::ios::binary);
- oss << sk.to_string(false, true, true, true);
- for (int i = 25; i < (1 << 20); ++i) { sk.update(i); }
- oss << sk.to_string(false, true, true, true);
- oss << sk.to_string(false, true, true, false);
-
- sk = hll_sketch_test_alloc(8, HLL_8);
- for (int i = 0; i < 25; ++i) { sk.update(i); }
- oss << sk.to_string(false, true, true, true);
- }
- REQUIRE(test_allocator_total_bytes == 0);
+ hll_sketch sk(15, HLL_4);
Review comment:
Fair. Gigantic PRs are obnoxious in many ways and we don't need them all to do everything. Thanks.
----------------------------------------------------------------
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 #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#issuecomment-777881282
## Pull Request Test Coverage Report for [Build 559502607](https://coveralls.io/builds/37075112)
* **0** of **0** changed or added relevant lines in **0** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.2%**) to **94.522%**
---
| Totals | [](https://coveralls.io/builds/37075112) |
| :-- | --: |
| Change from base [Build 556164680](https://coveralls.io/builds/37039799): | 0.2% |
| Covered Lines: | 4417 |
| Relevant Lines: | 4673 |
---
##### 💛 - [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 a change in pull request #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#discussion_r575424041
##########
File path: sampling/test/var_opt_allocation_test.cpp
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <var_opt_sketch.hpp>
+#include <var_opt_union.hpp>
+#include <test_type.hpp>
+#include <test_allocator.hpp>
+
+#include <catch.hpp>
+
+#include <sstream>
+
+namespace datasketches {
+
+using var_opt_test_sketch = var_opt_sketch<test_type, test_type_serde, test_allocator<test_type>>;
+using var_opt_test_union = var_opt_union<test_type, test_type_serde, test_allocator<test_type>>;
+
+TEST_CASE("varopt allocation test", "[var_opt_sketch]") {
+ test_allocator_total_bytes = 0;
+ test_allocator_net_allocations = 0;
+ {
+ var_opt_test_sketch sk1(10, var_opt_test_sketch::DEFAULT_RESIZE_FACTOR, 0);
+ for (int i = 0; i < 100; ++i) sk1.update(i);
+ auto bytes1 = sk1.serialize();
+ auto sk2 = var_opt_test_sketch::deserialize(bytes1.data(), bytes1.size(), 0);
+
+ std::stringstream ss;
+ sk1.serialize(ss);
+ auto sk3 = var_opt_test_sketch::deserialize(ss, 0);
+
+ var_opt_test_union u1(10, 0);
+ u1.update(sk1);
+ u1.update(sk2);
+ u1.update(sk3);
+
+ auto bytes2 = u1.serialize();
+ auto u2 = var_opt_test_union::deserialize(bytes2.data(), bytes2.size(), 0);
+ }
+ REQUIRE(test_allocator_total_bytes == 0);
+ REQUIRE(test_allocator_net_allocations == 0);
+}
+
+TEST_CASE( "varopt union: move", "[var_opt_union][test_type]") {
+ uint32_t n = 20;
+ uint32_t k = 5;
+ var_opt_union<test_type> u(k);
+ var_opt_sketch<test_type> sk1(k);
+ var_opt_sketch<test_type> sk2(k);
+
+ // move udpates
+ for (int i = 0; i < (int) n; ++i) {
+ sk1.update(i);
+ sk2.update(-i);
+ }
+ REQUIRE(sk1.get_n() == n);
+ REQUIRE(sk2.get_n() == n);
+
+ // move unions
+ u.update(std::move(sk2));
+ u.update(std::move(sk1));
+ REQUIRE(u.get_result().get_n() == 2 * n);
+
+ // move constructor
+ var_opt_union<test_type> u2(std::move(u));
+ REQUIRE(u2.get_result().get_n() == 2 * n);
+
+ // move assignment
+ var_opt_union<test_type> u3(k);
+ u3 = std::move(u2);
+ REQUIRE(u3.get_result().get_n() == 2 * n);
Review comment:
Got it. Thanks.
----------------------------------------------------------------
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 #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
jmalkin commented on pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#issuecomment-778344624
I guess the idea of tutorial tests is probably outside the scope of this specific PR.
----------------------------------------------------------------
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 #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov merged pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195
----------------------------------------------------------------
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 #195: test that allocator default constructor is not called
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #195:
URL: https://github.com/apache/datasketches-cpp/pull/195#discussion_r575407963
##########
File path: hll/test/HllSketchTest.cpp
##########
@@ -162,27 +162,23 @@ TEST_CASE("hll sketch: check ser sizes", "[hll_sketch]") {
}
TEST_CASE("hll sketch: exercise to string", "[hll_sketch]") {
- test_allocator_total_bytes = 0;
- {
- hll_sketch_test_alloc sk(15, HLL_4);
- for (int i = 0; i < 25; ++i) { sk.update(i); }
- std::ostringstream oss(std::ios::binary);
- oss << sk.to_string(false, true, true, true);
- for (int i = 25; i < (1 << 20); ++i) { sk.update(i); }
- oss << sk.to_string(false, true, true, true);
- oss << sk.to_string(false, true, true, false);
-
- sk = hll_sketch_test_alloc(8, HLL_8);
- for (int i = 0; i < 25; ++i) { sk.update(i); }
- oss << sk.to_string(false, true, true, true);
- }
- REQUIRE(test_allocator_total_bytes == 0);
+ hll_sketch sk(15, HLL_4);
Review comment:
Yes, I thought about splitting this set of tests into simple tests that can serve as usage examples, and more elaborate allocation tests with this additional clutter (passing allocators, checking allocated bytes and so on). I am afraid it is a bit more involved than I planned at this moment.
----------------------------------------------------------------
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