Revert #476: remove the array-of-strings tuple sketch for 5.3.0 - #537
Merged
Merged
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The generic array revert introduces missing-header, moved-from-state, and unsafe equality regressions.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Removes the unreleased array-of-strings tuple sketch to avoid shipping a format incompatible with Java.
Changes:
- Deletes the AoS implementation and tests.
- Removes AoS build and installation entries.
- Reverts related generic array changes.
| File | Description |
|---|---|
tuple/CMakeLists.txt |
Stops installing AoS headers. |
tuple/include/array_of_strings_sketch.hpp |
Removes the public AoS API. |
tuple/include/array_of_strings_sketch_impl.hpp |
Removes the AoS implementation. |
tuple/include/array_tuple_sketch.hpp |
Reverts generic array changes. |
tuple/test/CMakeLists.txt |
Removes AoS test targets. |
tuple/test/array_of_strings_sketch_test.cpp |
Removes unit tests. |
tuple/test/aos_sketch_serialize_for_java.cpp |
Removes Java serialization fixtures. |
tuple/test/aos_sketch_deserialize_from_java_test.cpp |
Removes Java compatibility tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This reverts commit 1a23698, reversing changes made to f546262, except for tuple/include/array_tuple_sketch.hpp. The array-of-strings tuple sketch hashes keys as UTF-8, while Java hashes them as UTF-16LE, so its sketches cannot be combined with Java's (#533). It has not been released yet, so it is removed for 5.3.0 and will be rewritten once the encoding is agreed on dev@. The generic array<T> changes from #476 in array_tuple_sketch.hpp are kept: the <algorithm> include, element construction and destruction for non-trivial types, resetting the size of a moved-from array, and the size check in operator==. They are not specific to array-of-strings, and the array-of-doubles sketch uses the same class. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
leerho
force-pushed
the
revert-aos-sketch
branch
from
October 3, 2026 19:19
ed28012 to
b3c7a81
Compare
proost
approved these changes
Oct 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
array<T>improvements inarray_tuple_sketch.hpp: the<algorithm>include, element construction and destruction for non-trivial types, resetting the size of a moved-from array, and the size check inoperator==. They aren't AoS-specific, and the array-of-doubles sketch uses the same class.🤖 Generated with Claude Code