You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "leerho (via GitHub)" <gi...@apache.org> on 2026/10/03 19:01:09 UTC
[PR] Revert #476: remove the array-of-strings tuple sketch for 5.3.0 [datasketches-cpp]
leerho opened a new pull request, #537:
URL: https://github.com/apache/datasketches-cpp/pull/537
Reverts #476, as agreed with @proost on #533.
The C++ array-of-strings (AoS) tuple sketch hashes keys as UTF-8, while Java's AoS sketch hashes them as UTF-16LE, so the two produce different hashes for the same keys and their sketches can't be combined correctly (#533). The C++ AoS sketch has never been released, so removing it now affects no users and keeps 5.3.0 from shipping an incompatible format.
The encoding policy is being discussed on dev@: https://lists.apache.org/thread/5vlbnodmw8h5s0oc1988f4pdhkjnv8qw. The sketch will be rewritten once that's agreed.
- Removes the 1,177 lines #476 added and restores the 4 lines it changed. Nothing else in the repo depends on the AoS code.
- All 17 test suites pass locally.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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
Re: [PR] Revert #476: remove the array-of-strings tuple sketch for 5.3.0 [datasketches-cpp]
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #537:
URL: https://github.com/apache/datasketches-cpp/pull/537#discussion_r4174500771
##########
tuple/include/array_tuple_sketch.hpp:
##########
@@ -36,32 +34,27 @@ class array {
public:
using value_type = T;
using allocator_type = Allocator;
- using alloc_traits = std::allocator_traits<Allocator>;
- explicit array(uint8_t size, const T& value, const Allocator& allocator = Allocator()):
+ explicit array(uint8_t size, T value, const Allocator& allocator = Allocator()):
allocator_(allocator), size_(size), array_(allocator_.allocate(size_)) {
- init_values(value, std::is_trivially_copyable<T>());
+ std::fill(array_, array_ + size_, value);
Review Comment:
Agreed, and the same applies to the other generic `array<T>` changes from #476 (moved-from size reset, the size check in `operator==`, element construction/destruction). None are AoS-specific, and array-of-doubles uses the same class. The revert now keeps `array_tuple_sketch.hpp` exactly as it is on master and only removes the AoS files.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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
Re: [PR] Revert #476: remove the array-of-strings tuple sketch for 5.3.0 [datasketches-cpp]
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho merged PR #537:
URL: https://github.com/apache/datasketches-cpp/pull/537
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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
Re: [PR] Revert #476: remove the array-of-strings tuple sketch for 5.3.0 [datasketches-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #537:
URL: https://github.com/apache/datasketches-cpp/pull/537#discussion_r4174458321
##########
tuple/include/array_tuple_sketch.hpp:
##########
@@ -36,32 +34,27 @@ class array {
public:
using value_type = T;
using allocator_type = Allocator;
- using alloc_traits = std::allocator_traits<Allocator>;
- explicit array(uint8_t size, const T& value, const Allocator& allocator = Allocator()):
+ explicit array(uint8_t size, T value, const Allocator& allocator = Allocator()):
allocator_(allocator), size_(size), array_(allocator_.allocate(size_)) {
- init_values(value, std::is_trivially_copyable<T>());
+ std::fill(array_, array_ + size_, value);
Review Comment:
`std::fill` (and the copy constructor's `std::copy`) is declared by `<algorithm>`, but this patch removes that direct include. The public header can therefore fail when included by a translation unit whose other headers do not expose those names transitively; keep `<algorithm>` even though the string-specific support is being reverted.
This issue also appears in the following locations of the same file:
- line 54
- line 77
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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
Re: [PR] Revert #476: remove the array-of-strings tuple sketch for 5.3.0 [datasketches-cpp]
Posted by "coveralls (via GitHub)" <gi...@apache.org>.
coveralls commented on PR #537:
URL: https://github.com/apache/datasketches-cpp/pull/537#issuecomment-5972530452
## Coverage Report for [CI Build 37146373509](https://coveralls.io/builds/82132481)
### Coverage decreased (**-0.1%**) to **82.327%**
### Details
- Coverage decreased (-0.1%) from the [base build](https://coveralls.io/builds/82075580).
- **Patch coverage**: **4** of **4** lines across **1** file are fully covered (100%).
- **11 coverage regressions** across **2** files.
### Uncovered Changes
No uncovered changes found.
### Coverage Regressions
**11** previously-covered lines in **2** files lost coverage.
| File | Lines Losing Coverage | Coverage |
| :--- | ---------: | -------: |
| [build/_deps/catch2-src/single_include/catch2/catch.hpp](https://coveralls.io/builds/82132481/source?filename=build%2F_deps%2Fcatch2-src%2Fsingle_include%2Fcatch2%2Fcatch.hpp#L11751) | 7 | 34.91% |
| [tuple/include/tuple_sketch_impl.hpp](https://coveralls.io/builds/82132481/source?filename=tuple%2Finclude%2Ftuple_sketch_impl.hpp#L428) | 4 | 95.54% |
---
### Coverage Stats
| | [](https://coveralls.io/builds/82132481) |
| :-- | --: |
| Relevant Lines: | 21388 |
| Covered Lines: | 17608 |
| Line Coverage: | 82.33% |
| Coverage Strength: | 1374195.81 hits per line |
---
##### 💛 - [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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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