Skip to content

Filter compression schemes by serialized IDs and upgrade decimal modes #9992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mhk197 wants to merge 6 commits into from
Closed

Filter compression schemes by serialized IDs and upgrade decimal modes #9992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2add9d7
Configure compressor schemes from serialized ID permissions
mhk197 Sep 22, 2026
ee65f48
Configure compression schemes through serialized ID permissions
mhk197 Sep 22, 2026
ad9a428
Preserve declared encodings and upgrade decimal schemes explicitly
mhk197 Sep 22, 2026
259a795
Initialize compressor permissions from serialized IDs
mhk197 Sep 23, 2026
95e88c9
Centralize edition ID resolution and fix scheme configuration checks
mhk197 Sep 23, 2026
0c415a7
Add builder methods to replace and extend permitted encodings
mhk197 Sep 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions benchmarks/compress-bench/src/gpu/vortex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ use vortex::array::IntoArray;
use vortex::array::VortexSessionExecute;
use vortex::array::arrays::StructArray;
use vortex::array::arrays::struct_::StructArrayExt;
use vortex::compressor::BtrBlocksCompressorBuilder;
use vortex::error::VortexResult;
use vortex::file::OpenOptionsSessionExt;
use vortex::file::WriteOptionsSessionExt;
Expand All @@ -36,8 +35,8 @@ use vortex_bench::compress::Compressed;
use vortex_bench::compress::CompressedData;
use vortex_bench::compress::Compressor;
use vortex_bench::compress::Uncompressed;
use vortex_bench::compressor_builder_for_session;
use vortex_bench::conversions::parquet_to_vortex_chunks_with_batch_size;
use vortex_bench::retain_edition_encodings;
use vortex_cuda::CanonicalCudaExt;
use vortex_cuda::CudaExecutionCtx;
use vortex_cuda::CudaOpenOptionsExt;
Expand Down Expand Up @@ -100,11 +99,9 @@ impl Compressor for GpuVortexCompressor {
// partition rather than whatever the default strategy would regroup them into.
let strategy = Arc::new(ChunkedLayoutStrategy::new(CompressingStrategy::new(
CudaFlatLayoutStrategy::default(),
retain_edition_encodings(
&SESSION,
BtrBlocksCompressorBuilder::default().only_cuda_compatible(),
)
.build(),
compressor_builder_for_session(&SESSION)
.only_cuda_compatible()
.build(),
)));
let start = Instant::now();
SESSION
Expand Down
11 changes: 4 additions & 7 deletions vortex-bench/src/conversions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ use vortex::array::arrays::struct_::StructArrayExt;
use vortex::array::builders::builder_with_capacity_in;
use vortex::array::stream::ArrayStreamAdapter;
use vortex::array::stream::ArrayStreamExt;
use vortex::compressor::BtrBlocksCompressorBuilder;
use vortex::dtype::DType;
use vortex::dtype::FieldPath;
use vortex::dtype::StructFields;
Expand Down Expand Up @@ -66,7 +65,7 @@ use wkb::writer::write_geometry;
use crate::CompactionStrategy;
use crate::Format;
use crate::SESSION;
use crate::retain_edition_encodings;
use crate::compressor_builder_for_session;
use crate::utils::file::idempotent_async;

/// Memory budget per concurrent conversion stream in GB. This is somewhat arbitary.
Expand Down Expand Up @@ -248,10 +247,8 @@ fn write_options_for(

let mut builder = WriteStrategyBuilder::default();
if matches!(compaction, CompactionStrategy::Compact) {
builder = builder.with_btrblocks_builder(retain_edition_encodings(
&SESSION,
BtrBlocksCompressorBuilder::default().with_compact(),
));
builder =
builder.with_btrblocks_builder(compressor_builder_for_session(&SESSION).with_compact());
}
for name in binary_fields {
builder = builder.with_field_writer(FieldPath::from_name(name), no_dict_layout());
Expand All @@ -263,7 +260,7 @@ fn write_options_for(
fn no_dict_layout() -> Arc<dyn LayoutStrategy> {
Arc::new(CompressingStrategy::new(
ChunkedLayoutStrategy::new(FlatLayoutStrategy::default()),
retain_edition_encodings(&SESSION, BtrBlocksCompressorBuilder::default()).build(),
compressor_builder_for_session(&SESSION).build(),
))
}

Expand Down
18 changes: 6 additions & 12 deletions vortex-bench/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,30 +255,24 @@ impl CompactionStrategy {
match self {
CompactionStrategy::Compact => options.with_strategy(
WriteStrategyBuilder::default()
.with_btrblocks_builder(retain_edition_encodings(
&SESSION,
BtrBlocksCompressorBuilder::default().with_compact(),
))
.with_btrblocks_builder(compressor_builder_for_session(&SESSION).with_compact())
.build(),
),
CompactionStrategy::Default => options,
}
}
}

/// Restrict `builder` to the encodings permitted by the session's enabled editions.
/// Create a compressor builder permitting the session's enabled array encodings.
///
/// The default writer applies this filter itself. An explicit strategy bypasses it, so a
/// benchmark that builds its own compressor applies it here to stay within editions.
pub fn retain_edition_encodings(
session: &VortexSession,
builder: BtrBlocksCompressorBuilder,
) -> BtrBlocksCompressorBuilder {
/// Benchmarks supplying an explicit strategy use the session's permissions, including opt-in
/// editions, instead of the compressor builder's default core edition.
pub fn compressor_builder_for_session(session: &VortexSession) -> BtrBlocksCompressorBuilder {
let allowed = session
.enabled_component_ids(ComponentKind::Array)
.into_iter()
.collect();
builder.retain_allowed_encodings(&allowed)
BtrBlocksCompressorBuilder::new(allowed)
}

/// Verify that local data has already been prepared for the requested benchmark formats.
Expand Down
2 changes: 1 addition & 1 deletion vortex-btrblocks/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ vortex-buffer = { workspace = true }
vortex-compressor = { workspace = true }
vortex-datetime-parts = { workspace = true }
vortex-decimal-byte-parts = { workspace = true }
vortex-edition = { workspace = true }

@joseph-isaacs joseph-isaacs Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this? Ideally you pass id on construction.

Compressor shouldn't need to know about edition just allowed ids

@robert3005 robert3005 Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is because of Default impl. I think you can construct the compressor and then only filter it in the write strategy

@mhk197 mhk197 Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is because of Default, which I thought we didn't want to completely break.

In order to avoid this we either need to:

  1. Remove Default and have compressor constructors always take a permitted id list.
  2. No ID is permitted to default compressor, and you set them later
  3. A subset of the IDs in ALL_SCHEMES are permitted in default compressor via a default allowlist.

I prefer 1 because 2 is effectively a hard break and 3 requires more bookkeeping

@robert3005 robert3005 Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can have unfiltered list here that needs to have ids supplied during construction?

@robert3005 robert3005 Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE we postfilter

@mhk197 mhk197 Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would not solve the delta problem we had though iiuc

@robert3005 robert3005 Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta is not in any edition so it would get removed from the supported strategies

vortex-error = { workspace = true }
vortex-fastlanes = { workspace = true }
vortex-fsst = { workspace = true }
Expand All @@ -49,7 +50,6 @@ tpchgen = { workspace = true }
tpchgen-arrow = { workspace = true }
vortex-array = { workspace = true, features = ["_test-harness"] }
vortex-arrow = { workspace = true }
vortex-edition = { workspace = true }
vortex-mask = { workspace = true }
vortex-session = { workspace = true }

Expand Down
Loading
Loading