Skip to content

Commit a6513a0

Browse files
fix(layout/dict): probe with the configured compressor instead of a hardcoded default (#8406)
## Summary Closes: #8405 `DictStrategy`'s dict-fit probe was hardcoded to `BtrBlocksCompressor::default()`, ignoring the caller's configured compressor. The probe now takes `stats_compressor` as a parameter to `DictStrategy::new`, so caller scheme exclusions are honoured. `stats_compressor` rather than `data_compressor` because the probe needs all dictionary schemes to detect eligibility. `data_compressor` excludes `IntDictScheme`. ## Testing New test `dict_probe_honours_configured_compressor`: asserts dict layout with the default builder, asserts no dict layout with `StringDictScheme` excluded. AI use disclosure: fix authored with assistance from Claude Code. --------- Signed-off-by: Thomas Santerre <thomas@santerre.xyz> Signed-off-by: Onur Satici <onursatici@users.noreply.github.com> Co-authored-by: Onur Satici <onursatici@users.noreply.github.com>
1 parent 452e0f8 commit a6513a0

4 files changed

Lines changed: 109 additions & 3 deletions

File tree

vortex-file/src/strategy.rs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ pub struct WriteStrategyBuilder {
156156
field_writers: HashMap<FieldPath, Arc<dyn LayoutStrategy>>,
157157
allow_encodings: Option<HashSet<ArrayId>>,
158158
flat_strategy: Option<Arc<dyn LayoutStrategy>>,
159+
probe_compressor: Option<Arc<dyn CompressorPlugin>>,
159160
}
160161

161162
impl Default for WriteStrategyBuilder {
@@ -168,6 +169,7 @@ impl Default for WriteStrategyBuilder {
168169
field_writers: HashMap::new(),
169170
allow_encodings: Some(ALLOWED_ENCODINGS.clone()),
170171
flat_strategy: None,
172+
probe_compressor: None,
171173
}
172174
}
173175
}
@@ -231,6 +233,12 @@ impl WriteStrategyBuilder {
231233
self
232234
}
233235

236+
/// Override the compressor used to probe whether a column is dict-eligible.
237+
pub fn with_probe_compressor<C: CompressorPlugin>(mut self, compressor: C) -> Self {
238+
self.probe_compressor = Some(Arc::new(compressor));
239+
self
240+
}
241+
234242
/// Builds the canonical [`LayoutStrategy`] implementation, with the configured overrides
235243
/// applied.
236244
pub fn build(self) -> Arc<dyn LayoutStrategy> {
@@ -284,14 +292,20 @@ impl WriteStrategyBuilder {
284292
CompressorConfig::BtrBlocks(builder) => Arc::new(builder.build()),
285293
CompressorConfig::Opaque(compressor) => compressor,
286294
};
287-
let compress_then_flat = CompressingStrategy::new(flat, stats_compressor);
295+
let compress_then_flat = CompressingStrategy::new(flat, Arc::clone(&stats_compressor));
288296

289297
// 3. apply dict encoding or fallback
298+
let probe_compressor = if let Some(probe_compressor) = self.probe_compressor {
299+
probe_compressor
300+
} else {
301+
Arc::clone(&stats_compressor)
302+
};
290303
let dict = DictStrategy::new(
291304
coalescing.clone(),
292305
compress_then_flat.clone(),
293306
coalescing,
294307
Default::default(),
308+
probe_compressor,
295309
);
296310

297311
// 2. calculate stats for each row group

vortex-file/src/tests.rs

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ use vortex_array::stats::PRUNING_STATS;
5858
use vortex_array::stream::ArrayStreamAdapter;
5959
use vortex_array::stream::ArrayStreamExt;
6060
use vortex_array::validity::Validity;
61+
use vortex_btrblocks::BtrBlocksCompressorBuilder;
62+
use vortex_btrblocks::SchemeExt;
63+
use vortex_btrblocks::schemes::string::StringDictScheme;
6164
use vortex_buffer::Buffer;
6265
use vortex_buffer::ByteBufferMut;
6366
use vortex_buffer::buffer;
@@ -1817,6 +1820,16 @@ fn assert_offsets_ordered(before: &[u64], after: &[u64], context: &str) {
18171820
}
18181821
}
18191822

1823+
/// Whether any node in the layout tree is a dict layout.
1824+
fn layout_has_dict(layout: &dyn Layout) -> bool {
1825+
layout.encoding_id().as_ref() == "vortex.dict"
1826+
|| layout
1827+
.children()
1828+
.unwrap()
1829+
.iter()
1830+
.any(|child| layout_has_dict(child.as_ref()))
1831+
}
1832+
18201833
/// Mirrors the (private) `IDEAL_SPLIT_SIZE` that `SplitBy::Layout` uses to sub-divide wide
18211834
/// chunk-boundary spans: layout splits are never wider than this many rows.
18221835
const MAX_SPLIT_ROWS: u64 = 100_000;
@@ -2003,6 +2016,75 @@ async fn test_segment_ordering_dict_codes_before_values() -> VortexResult<()> {
20032016
Ok(())
20042017
}
20052018

2019+
#[tokio::test]
2020+
#[cfg_attr(miri, ignore)]
2021+
async fn dict_probe_honours_configured_compressor() -> VortexResult<()> {
2022+
// Low-cardinality strings so the default cascade picks a dictionary.
2023+
let n = 32_768;
2024+
let values: Vec<&str> = (0..n).map(|i| ["alpha", "beta", "gamma"][i % 3]).collect();
2025+
let strings = VarBinArray::from(values).into_array();
2026+
2027+
let mut buf = ByteBufferMut::empty();
2028+
let summary = SESSION
2029+
.write_options()
2030+
.with_strategy(crate::strategy::WriteStrategyBuilder::default().build())
2031+
.write(&mut buf, strings.clone().to_array_stream())
2032+
.await?;
2033+
assert!(
2034+
layout_has_dict(summary.footer().layout().as_ref()),
2035+
"default builder should produce a dict layout for low-cardinality strings"
2036+
);
2037+
2038+
let no_string_dict =
2039+
BtrBlocksCompressorBuilder::default().exclude_schemes([StringDictScheme.id()]);
2040+
let mut buf = ByteBufferMut::empty();
2041+
let summary = SESSION
2042+
.write_options()
2043+
.with_strategy(
2044+
crate::strategy::WriteStrategyBuilder::default()
2045+
.with_btrblocks_builder(no_string_dict)
2046+
.build(),
2047+
)
2048+
.write(&mut buf, strings.to_array_stream())
2049+
.await?;
2050+
assert!(
2051+
!layout_has_dict(summary.footer().layout().as_ref()),
2052+
"excluding StringDict from the configured compressor should disable the dict layout"
2053+
);
2054+
2055+
Ok(())
2056+
}
2057+
2058+
#[tokio::test]
2059+
#[cfg_attr(miri, ignore)]
2060+
async fn probe_compressor_override_is_independent() -> VortexResult<()> {
2061+
// Low-cardinality strings the default cascade would dict-encode.
2062+
let n = 32_768;
2063+
let values: Vec<&str> = (0..n).map(|i| ["alpha", "beta", "gamma"][i % 3]).collect();
2064+
let strings = VarBinArray::from(values).into_array();
2065+
2066+
let probe_without_dict = BtrBlocksCompressorBuilder::default()
2067+
.exclude_schemes([StringDictScheme.id()])
2068+
.build();
2069+
2070+
let mut buf = ByteBufferMut::empty();
2071+
let summary = SESSION
2072+
.write_options()
2073+
.with_strategy(
2074+
crate::strategy::WriteStrategyBuilder::default()
2075+
.with_probe_compressor(probe_without_dict)
2076+
.build(),
2077+
)
2078+
.write(&mut buf, strings.to_array_stream())
2079+
.await?;
2080+
assert!(
2081+
!layout_has_dict(summary.footer().layout().as_ref()),
2082+
"probe override should disable the dict layout independently of the data/stats compressor"
2083+
);
2084+
2085+
Ok(())
2086+
}
2087+
20062088
#[tokio::test]
20072089
#[cfg_attr(miri, ignore)]
20082090
async fn test_segment_ordering_zonemaps_after_data() -> VortexResult<()> {

vortex-layout/src/layouts/dict/reader.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,7 @@ mod tests {
360360
use vortex_array::expr::pack;
361361
use vortex_array::expr::root;
362362
use vortex_array::validity::Validity;
363+
use vortex_btrblocks::BtrBlocksCompressor;
363364
use vortex_error::VortexExpect;
364365
use vortex_error::VortexResult;
365366
use vortex_io::runtime::Handle;
@@ -401,6 +402,7 @@ mod tests {
401402
FlatLayoutStrategy::default(),
402403
FlatLayoutStrategy::default(),
403404
DictLayoutOptions::default(),
405+
Arc::new(BtrBlocksCompressor::default()),
404406
);
405407
let segments = Arc::new(TestSegments::default());
406408
let (ptr, eof) = SequenceId::root().split();
@@ -430,6 +432,7 @@ mod tests {
430432
FlatLayoutStrategy::default(),
431433
FlatLayoutStrategy::default(),
432434
DictLayoutOptions::default(),
435+
Arc::new(BtrBlocksCompressor::default()),
433436
);
434437

435438
let array = VarBinArray::from_iter(
@@ -530,6 +533,7 @@ mod tests {
530533
FlatLayoutStrategy::default(),
531534
FlatLayoutStrategy::default(),
532535
DictLayoutOptions::default(),
536+
Arc::new(BtrBlocksCompressor::default()),
533537
);
534538

535539
let array =
@@ -582,6 +586,7 @@ mod tests {
582586
FlatLayoutStrategy::default(),
583587
FlatLayoutStrategy::default(),
584588
DictLayoutOptions::default(),
589+
Arc::new(BtrBlocksCompressor::default()),
585590
);
586591

587592
let array = VarBinArray::from_iter(

vortex-layout/src/layouts/dict/writer.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ use vortex_array::builders::dict::dict_encoder;
3030
use vortex_array::dtype::DType;
3131
use vortex_array::dtype::Nullability;
3232
use vortex_array::dtype::PType;
33-
use vortex_btrblocks::BtrBlocksCompressor;
3433
use vortex_error::VortexError;
3534
use vortex_error::VortexExpect;
3635
use vortex_error::VortexResult;
@@ -44,6 +43,7 @@ use crate::LayoutRef;
4443
use crate::LayoutStrategy;
4544
use crate::OwnedLayoutChildren;
4645
use crate::layouts::chunked::ChunkedLayout;
46+
use crate::layouts::compressed::CompressorPlugin;
4747
use crate::layouts::dict::DictLayout;
4848
use crate::segments::SegmentSinkRef;
4949
use crate::sequence::SendableSequentialStream;
@@ -108,6 +108,7 @@ pub struct DictStrategy {
108108
values: Arc<dyn LayoutStrategy>,
109109
fallback: Arc<dyn LayoutStrategy>,
110110
options: DictLayoutOptions,
111+
probe_compressor: Arc<dyn CompressorPlugin>,
111112
}
112113

113114
impl DictStrategy {
@@ -116,12 +117,14 @@ impl DictStrategy {
116117
values: Values,
117118
fallback: Fallback,
118119
options: DictLayoutOptions,
120+
probe_compressor: Arc<dyn CompressorPlugin>,
119121
) -> Self {
120122
Self {
121123
codes: Arc::new(codes),
122124
values: Arc::new(values),
123125
fallback: Arc::new(fallback),
124126
options,
127+
probe_compressor,
125128
}
126129
}
127130
}
@@ -155,7 +158,9 @@ impl LayoutStrategy for DictStrategy {
155158
None => true, // empty stream
156159
Some(chunk) => {
157160
let mut exec_ctx = session.create_execution_ctx();
158-
let compressed = BtrBlocksCompressor::default().compress(&chunk, &mut exec_ctx)?;
161+
let compressed = self
162+
.probe_compressor
163+
.compress_chunk(&chunk, &mut exec_ctx)?;
159164
!compressed.is::<Dict>()
160165
}
161166
};

0 commit comments

Comments
 (0)