Skip to content

Commit d228008

Browse files
committed
fix
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
1 parent 6d62690 commit d228008

5 files changed

Lines changed: 135 additions & 11 deletions

File tree

‎vortex-btrblocks/src/builder/tests.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,8 @@ fn restrictions_select_predecessors_of_schemes_added_later() {
200200
.with_new_scheme(&NewFoRScheme)
201201
.build();
202202
assert!(compressor.has_scheme(integer::FoRScheme.id()));
203-
assert!(compressor.has_scheme(NewFoRScheme.id()));
203+
assert!(compressor.has_scheme_family(NewFoRScheme.id()));
204+
assert!(!compressor.has_scheme(NewFoRScheme.id()));
204205
}
205206

206207
#[rstest]
@@ -211,6 +212,6 @@ fn excluding_any_version_removes_the_chain(#[case] excluded: SchemeId) {
211212
.with_new_scheme(&NewFoRScheme)
212213
.exclude_schemes([excluded])
213214
.build();
214-
assert!(!compressor.has_scheme(integer::FoRScheme.id()));
215-
assert!(!compressor.has_scheme(NewFoRScheme.id()));
215+
assert!(!compressor.has_scheme_family(integer::FoRScheme.id()));
216+
assert!(!compressor.has_scheme_family(NewFoRScheme.id()));
216217
}
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// SPDX-FileCopyrightText: Copyright the Vortex contributors
3+
4+
#![cfg(feature = "unstable_encodings")]
5+
6+
#[cfg(test)]
7+
mod tests {
8+
use rstest::rstest;
9+
use vortex_array::ArrayId;
10+
use vortex_array::ArrayRef;
11+
use vortex_array::Canonical;
12+
use vortex_array::ExecutionCtx;
13+
use vortex_array::IntoArray;
14+
use vortex_array::VTable;
15+
use vortex_array::VortexSessionExecute;
16+
use vortex_array::array_session;
17+
use vortex_array::arrays::PrimitiveArray;
18+
use vortex_array::assert_arrays_eq;
19+
use vortex_btrblocks::ArrayAndStats;
20+
use vortex_btrblocks::CascadingCompressor;
21+
use vortex_btrblocks::CompressorContext;
22+
use vortex_btrblocks::Scheme;
23+
use vortex_btrblocks::SchemeExt;
24+
use vortex_btrblocks::schemes::integer::DeltaScheme;
25+
use vortex_btrblocks::schemes::integer::IntRLEScheme;
26+
use vortex_compressor::scheme::CompressionEstimate;
27+
use vortex_compressor::scheme::EstimateVerdict;
28+
use vortex_error::VortexResult;
29+
use vortex_fastlanes::Delta;
30+
use vortex_fastlanes::RLE;
31+
use vortex_session::registry::CachedId;
32+
33+
static DELTA_V2_ID: CachedId = CachedId::new("test.delta_v2");
34+
static DELTA_V1: DeltaScheme = DeltaScheme::new(1.25);
35+
36+
#[derive(Debug)]
37+
struct DeltaV2;
38+
39+
impl Scheme for DeltaV2 {
40+
fn scheme_name(&self) -> &'static str {
41+
"test.delta_v2"
42+
}
43+
44+
fn matches(&self, canonical: &Canonical) -> bool {
45+
DELTA_V1.matches(canonical)
46+
}
47+
48+
fn produced_encodings(&self) -> Vec<ArrayId> {
49+
vec![*DELTA_V2_ID]
50+
}
51+
52+
fn predecessor(&self) -> Option<&'static dyn Scheme> {
53+
Some(&DELTA_V1)
54+
}
55+
56+
fn num_children(&self) -> usize {
57+
2
58+
}
59+
60+
fn expected_compression_ratio(
61+
&self,
62+
_data: &ArrayAndStats,
63+
_compress_ctx: CompressorContext,
64+
_exec_ctx: &mut ExecutionCtx,
65+
) -> CompressionEstimate {
66+
CompressionEstimate::Verdict(EstimateVerdict::Skip)
67+
}
68+
69+
fn compress(
70+
&self,
71+
_compressor: &CascadingCompressor,
72+
data: &ArrayAndStats,
73+
_compress_ctx: CompressorContext,
74+
_exec_ctx: &mut ExecutionCtx,
75+
) -> VortexResult<ArrayRef> {
76+
Ok(data.array().clone())
77+
}
78+
}
79+
80+
#[rstest]
81+
#[case::predecessor(Delta.id(), true)]
82+
#[case::replacement(*DELTA_V2_ID, false)]
83+
fn rle_respects_selected_delta_version(
84+
#[case] allowed_delta: ArrayId,
85+
#[case] expect_delta: bool,
86+
) -> VortexResult<()> {
87+
let session = array_session();
88+
vortex_fastlanes::initialize(&session);
89+
let compressor = CascadingCompressor::new(vec![&IntRLEScheme, &DeltaV2])
90+
.with_allowed_serialized_ids([RLE.id(), allowed_delta].into_iter().collect());
91+
assert!(compressor.has_scheme_family(DELTA_V1.id()));
92+
let array = PrimitiveArray::from_iter((0..65_536u32).map(|i| (i / 64) % 100)).into_array();
93+
let mut ctx = session.create_execution_ctx();
94+
let compressed = compressor.compress(&array, &mut ctx)?;
95+
assert_eq!(compressed.encoding_id(), RLE.id());
96+
let has_delta = compressed
97+
.depth_first_traversal()
98+
.any(|array| array.encoding_id() == Delta.id());
99+
assert_eq!(has_delta, expect_delta);
100+
assert_arrays_eq!(compressed, array, &mut ctx);
101+
Ok(())
102+
}
103+
}

‎vortex-compressor/src/compressor/mod.rs‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ impl CascadingCompressor {
6363
/// Creates a new compressor with the given schemes.
6464
///
6565
/// Register only the newest version of each scheme. Predecessor IDs are aliases for the
66-
/// selected version in exclusions and [`has_scheme`](Self::has_scheme) checks.
66+
/// selected version in exclusions and [`has_scheme_family`](Self::has_scheme_family) checks.
6767
/// Root-level exclusion rules (e.g. excluding Dict from list offsets) are built automatically.
6868
///
6969
/// # Panics
@@ -146,14 +146,21 @@ impl CascadingCompressor {
146146
CompressorContext::new()
147147
}
148148

149-
/// Returns whether a version of `scheme` is enabled.
149+
/// Returns whether any version in the scheme's family is enabled.
150150
///
151-
/// Any ID in a registered predecessor chain refers to the selected version, including when
152-
/// the selected version is older or newer than the specified ID.
153-
pub fn has_scheme(&self, scheme: SchemeId) -> bool {
151+
/// A family is a registered scheme and its predecessor chain. `scheme` may name any version
152+
/// in that chain. Use [`Self::has_scheme`] to check the exact selected version.
153+
pub fn has_scheme_family(&self, scheme: SchemeId) -> bool {
154154
self.scheme_aliases.contains_key(&scheme)
155155
}
156156

157+
/// Returns whether this exact scheme version is selected for compression.
158+
///
159+
/// Use this before invoking a specific implementation directly.
160+
pub fn has_scheme(&self, scheme: SchemeId) -> bool {
161+
self.scheme_aliases.get(&scheme) == Some(&scheme)
162+
}
163+
157164
/// Resolves a registered version to the selected version, leaving unknown IDs unchanged.
158165
fn resolve_scheme_id(&self, scheme: SchemeId) -> SchemeId {
159166
self.scheme_aliases.get(&scheme).copied().unwrap_or(scheme)

‎vortex-compressor/src/compressor/version_tests.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,13 @@ fn newest_eligible_version_is_selected_before_estimation() -> VortexResult<()> {
152152
)?;
153153
assert_eq!(winner.map(|(scheme, _)| scheme.id()), Some(expected));
154154
for version in [&V1, &V2, &V3] {
155-
assert!(compressor.has_scheme(version.id()));
155+
assert!(compressor.has_scheme_family(version.id()));
156+
assert_eq!(
157+
compressor.has_scheme(version.id()),
158+
version.id() == expected
159+
);
156160
}
161+
assert!(!compressor.has_scheme(OTHER.id()));
157162
}
158163
Ok(())
159164
}
@@ -164,6 +169,7 @@ fn no_eligible_version_removes_the_entire_chain() {
164169
let compressor = CascadingCompressor::new(vec![&V3]).with_allowed_serialized_ids(allowed);
165170
assert!(compressor.schemes.is_empty());
166171
for version in [&V1, &V2, &V3] {
172+
assert!(!compressor.has_scheme_family(version.id()));
167173
assert!(!compressor.has_scheme(version.id()));
168174
}
169175
}
@@ -193,7 +199,7 @@ fn successive_restrictions_keep_aliases_and_intersect_wire_ids() {
193199

194200
let compressor = compressor.with_allowed_serialized_ids(HashSet::from([*V2_ID, *AUX_ID]));
195201
assert!(compressor.schemes.is_empty());
196-
assert!(!compressor.has_scheme(V3.id()));
202+
assert!(!compressor.has_scheme_family(V3.id()));
197203
}
198204

199205
static PUSH_OLD: TestScheme = TestScheme {

‎vortex-file/src/writer.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,14 @@ fn new_array_context(
402402
.registry()
403403
.read(|registry| registry.keys().copied().collect())
404404
};
405-
let allowed_serialized_ids: HashSet<ArrayId> = serialized_ids.iter().copied().collect();
405+
// Editions grant permission to use a wire format, but it also must be registered in the session.
406+
let allowed_serialized_ids = arrays.registry().read(|registry| {
407+
serialized_ids
408+
.iter()
409+
.copied()
410+
.filter(|id| registry.contains_key(id))
411+
.collect()
412+
});
406413
let array_ctx = ArrayContext::new(serialized_ids.iter().copied().sorted().collect());
407414
let array_ctx = if enforce_editions {
408415
// Only permit serialized IDs in the enabled editions.

0 commit comments

Comments
 (0)