diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aafd7baa..fcae6498 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -93,7 +93,7 @@ jobs: cache-bin: "false" - name: Prepare test data shell: bash - run: ./tools/generate_serialization_test_data.py + run: ./tools/download_serialization_test_data.py - name: Run unit tests shell: bash run: cargo x test diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4184e907..a5fe0696 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -88,7 +88,7 @@ cargo install taplo-cli typos-cli hawkeye Some tests depend on snapshot files under `datasketches/tests/serialization_test_data`. If they are missing, tests will fail. Download them with: ```shell -python3 ./tools/generate_serialization_test_data.py --all +python3 ./tools/download_serialization_test_data.py --all ``` The script downloads the latest snapshots from the `main` branch of @@ -100,8 +100,8 @@ The script downloads the latest snapshots from the `main` branch of You can download them separately: ```shell -python3 ./tools/generate_serialization_test_data.py --java -python3 ./tools/generate_serialization_test_data.py --cpp +python3 ./tools/download_serialization_test_data.py --java +python3 ./tools/download_serialization_test_data.py --cpp ``` The script requires Python 3 and network access. diff --git a/datasketches/src/tuple/hash_table.rs b/datasketches/src/tuple/hash_table.rs index 42b916aa..04775d7e 100644 --- a/datasketches/src/tuple/hash_table.rs +++ b/datasketches/src/tuple/hash_table.rs @@ -20,6 +20,8 @@ use std::num::NonZeroU64; use crate::thetacommon::RawHashTableEntry; use crate::thetacommon::hash_table::RawHashTable; +use crate::thetacommon::union::RawThetaUnionPolicy; +use crate::tuple::SummaryCombinePolicy; /// A retained entry in a Tuple sketch: a hash key together with its associated summary. #[derive(Debug, Clone)] @@ -37,7 +39,7 @@ impl TupleEntry { /// # Panics /// /// Panics if `hash` is zero. - pub(crate) fn new(hash: u64, summary: S) -> Self { + pub(super) fn new(hash: u64, summary: S) -> Self { let hash = NonZeroU64::new(hash).expect("hash must be non-zero"); Self { hash, summary } } @@ -51,11 +53,6 @@ impl TupleEntry { pub fn summary(&self) -> &S { &self.summary } - - /// Returns a mutable reference to the summary stored in this entry. - pub(super) fn summary_mut(&mut self) -> &mut S { - &mut self.summary - } } /// Specific hash table for tuple sketch. @@ -110,6 +107,12 @@ impl TupleHashTable { } } +impl RawThetaUnionPolicy> for P { + fn merge(&self, existing: &mut TupleEntry, incoming: TupleEntry) { + self.combine(&mut existing.summary, &incoming.summary); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/datasketches/src/tuple/union.rs b/datasketches/src/tuple/union.rs index 83ef8ec7..3267c546 100644 --- a/datasketches/src/tuple/union.rs +++ b/datasketches/src/tuple/union.rs @@ -29,25 +29,11 @@ use crate::thetacommon::constants::DEFAULT_LG_K; use crate::thetacommon::constants::MAX_LG_K; use crate::thetacommon::constants::MIN_LG_K; use crate::thetacommon::union::RawThetaUnion; -use crate::thetacommon::union::RawThetaUnionPolicy; use crate::tuple::hash_table::TupleEntry; use crate::tuple::policy::SummaryCombinePolicy; use crate::tuple::sketch::CompactTupleSketch; use crate::tuple::sketch::TupleSketchView; -/// Adapts a [`SummaryCombinePolicy`] to the raw union's entry-merge policy. -#[derive(Debug)] -struct CombinePolicyAdapter

(P); - -impl

RawThetaUnionPolicy> for CombinePolicyAdapter

-where - P: SummaryCombinePolicy, -{ - fn merge(&self, existing: &mut TupleEntry, incoming: TupleEntry) { - self.0.combine(existing.summary_mut(), incoming.summary()); - } -} - /// Union (set OR) of Tuple sketches. /// /// `P` is the [`SummaryCombinePolicy`] applied when a key is present in more than one input. For @@ -79,7 +65,7 @@ pub struct TupleUnion

where P: SummaryCombinePolicy, { - raw: RawThetaUnion, CombinePolicyAdapter

>, + raw: RawThetaUnion, P>, } impl

TupleUnion

@@ -217,7 +203,7 @@ where self.resize_factor, self.sampling_probability, self.seed, - CombinePolicyAdapter(self.policy), + self.policy, ), } } diff --git a/datasketches/tests/common.rs b/datasketches/tests/common.rs index e97b920a..ca26fe6e 100644 --- a/datasketches/tests/common.rs +++ b/datasketches/tests/common.rs @@ -39,10 +39,10 @@ pub fn serialization_test_data(sub_dir: &str, name: &str) -> PathBuf { r#"serialization test data file not found: {} Please ensure test data files are present in the repository. Generally, you can - run the following commands from the project root to regenerate the test data files + run the following commands from the project root to download the test data files if they are missing: - $ ./tools/generate_serialization_test_data.py + $ ./tools/download_serialization_test_data.py "#, path.display(), ); diff --git a/datasketches/tests/tuple_serialization_test.rs b/datasketches/tests/tuple_serialization_test.rs index ae692ee9..1de5f1e9 100644 --- a/datasketches/tests/tuple_serialization_test.rs +++ b/datasketches/tests/tuple_serialization_test.rs @@ -15,21 +15,6 @@ // specific language governing permissions and limitations // under the License. -//! Cross-language compatibility tests for Tuple sketch serialization. -//! -//! The fixtures are produced by the upstream Java and C++ generators (see -//! `tools/generate_serialization_test_data.py`): -//! -//! * Java: `TupleCrossLanguageTest.generateForCppIntegerSummary` writes `tuple_int_n{n}_java.sk` -//! using its `IntegerSummary`. -//! * C++: `tuple_sketch_serialize_for_java.cpp` writes `tuple_int_n{n}_cpp.sk` using an `int` -//! summary. -//! -//! Both build a tuple sketch with `update(i, i)` for `i` in `0..n`, so the summary is a 4-byte -//! little-endian signed integer — exactly what the `i32` [`TupleSummaryValue`] implementation -//! reads. The `aod_*`/`aos_*` fixtures use Array-of-Doubles / Array-of-Strings summaries, which -//! this crate does not implement, so they are intentionally not covered here. - #![cfg(feature = "tuple")] mod common; diff --git a/datasketches/tests/tuple_sketch_test.rs b/datasketches/tests/tuple_sketch_test.rs index e80b1552..fb5468e1 100644 --- a/datasketches/tests/tuple_sketch_test.rs +++ b/datasketches/tests/tuple_sketch_test.rs @@ -15,11 +15,6 @@ // specific language governing permissions and limitations // under the License. -//! Behavioral tests for the Tuple sketch, mirroring `theta_sketch_test.rs`. -//! -//! Updates carry a `u64` summary combined with the default (additive) policy, so the distinct-count -//! behavior matches the Theta sketch while the summaries accumulate alongside each key. - #![cfg(feature = "tuple")] use datasketches::common::NumStdDev; diff --git a/tools/generate_serialization_test_data.py b/tools/download_serialization_test_data.py similarity index 79% rename from tools/generate_serialization_test_data.py rename to tools/download_serialization_test_data.py index 2e9ba20a..ff522c25 100755 --- a/tools/generate_serialization_test_data.py +++ b/tools/download_serialization_test_data.py @@ -26,9 +26,7 @@ import zipfile from pathlib import Path, PurePosixPath -TCK_ARCHIVE_URL = ( - "https://github.com/apache/datasketches-tck/archive/refs/heads/main.zip" -) +TCK_ARCHIVE_URL = "https://github.com/apache/datasketches-tck/archive/0016a517/main.zip" OUTPUT_DIRS = { "java": "java_generated_files", "cpp": "cpp_generated_files", @@ -37,16 +35,13 @@ def download_archive(destination): print(f"Downloading serialization snapshots from {TCK_ARCHIVE_URL}", flush=True) - request = urllib.request.Request( - TCK_ARCHIVE_URL, - headers={"User-Agent": "apache-datasketches-rust"}, - ) + request = urllib.request.Request(TCK_ARCHIVE_URL) with urllib.request.urlopen(request, timeout=60) as response: with destination.open("wb") as output: shutil.copyfileobj(response, output) -def install_snapshots(archive, project_dir, language): +def extract_snapshots(archive, project_dir, language): source_parts = ("serialization", language, "snapshots") members = [] @@ -95,7 +90,7 @@ def install_snapshots(archive, project_dir, language): if temp_path is not None and temp_path.exists(): temp_path.unlink() - print(f"Installed {len(members)} {language} snapshots into {output_dir}") + print(f"Extracted {len(members)} {language} snapshots into {output_dir}") def main(): @@ -115,18 +110,15 @@ def main(): if not languages: languages = list(OUTPUT_DIRS) - project_dir = Path(__file__).resolve().parent.parent / "datasketches" - - try: - with tempfile.TemporaryDirectory(prefix="datasketches-tck-") as temp_dir: - archive_path = Path(temp_dir) / "datasketches-tck.zip" - download_archive(archive_path) - with zipfile.ZipFile(archive_path) as archive: - for language in languages: - install_snapshots(archive, project_dir, language) - except (OSError, RuntimeError, urllib.error.URLError, zipfile.BadZipFile) as error: - print(f"Error: {error}", file=sys.stderr) - return 1 + repository_root = Path(__file__).resolve().parents[1] + project_dir = repository_root / "datasketches" + + with tempfile.TemporaryDirectory(prefix="datasketches-tck-") as temp_dir: + archive_path = Path(temp_dir) / "datasketches-tck.zip" + download_archive(archive_path) + with zipfile.ZipFile(archive_path) as archive: + for language in languages: + extract_snapshots(archive, project_dir, language) return 0 diff --git a/typos.toml b/typos.toml index c9b22da2..1d84aebf 100644 --- a/typos.toml +++ b/typos.toml @@ -18,6 +18,7 @@ [default.extend-words] # False-Positive Abbreviations "PREINTS" = "PREINTS" +"SER" = "SER" [files] extend-exclude = []