-
Notifications
You must be signed in to change notification settings - Fork 187
ListLayout baseline
#8706
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.
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
Merged
Merged
ListLayout baseline
#8706
Changes from all commits
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
f0f1037
first pass
600aad7
fix
3bc9b10
second claude pass
0a1de6e
small fixes
a70fca7
fix
ab89456
fix
e3d7cb4
clean mod
mhk197 ae4ea06
fix writer
mhk197 bbfab05
fix writer
mhk197 12e5245
fix
mhk197 a79a508
improve projection eval
mhk197 a2232c5
projection evaluation
mhk197 1409d7a
tests
mhk197 ca3da81
tests
mhk197 1018ab6
fix test
mhk197 be2bc69
skip pruning eval
mhk197 61fe51f
few more tests
mhk197 ff93678
lint fix
mhk197 218f87e
fix test
mhk197 addabb8
quick fix
mhk197 e581fd3
add anylist matcher
mhk197 a09f615
read validity with all-true mask, not caller's mask
mhk197 4278dcf
narrow elements io for sparse mask instead of defering filtering
mhk197 e1f90ca
shortcut on whole-chunk unmasked reads
mhk197 18fa2f3
add required fallback to ListLayoutStrategy for non-list input
mhk197 119cd58
fmt
mhk197 bc986fa
cleanup
mhk197 d51b85e
fix rebase conflicts
mhk197 7fe6e59
use ListLayoutStrategy as default leaf under unstable_encodings
mhk197 494b421
recurse into nested lists
mhk197 a4d363f
fix: update ListLayout::build to use LayoutBuildContext after trait c…
mhk197 55797fe
comments
mhk197 6667caa
fix
mhk197 c98c35e
clean up writer
mhk197 101f2d4
Improve list reader
mhk197 4639437
Fix list docs
mhk197 f364166
Add list filter evaluation
mhk197 fa0fd05
Read list lengths from list layout offsets
mhk197 c5e1bf1
Move list expression planning to expr module
mhk197 84c4d7c
Fix list layout checks after rebase
mhk197 d79fd42
Remove list projection path labels
mhk197 3702457
Refine list expression child classification
mhk197 c3f2c91
remove stale comment
mhk197 571927c
simplify projection
mhk197 6074648
make list layout the default
mhk197 5a42bf9
list: top-level streaming decomposition behind VORTEX_EXPERIMENTAL_LI…
mhk197 de1e470
fix: drop redundant explicit rustdoc link target
mhk197 c8e02ab
list: simplify list layout reader/writer
mhk197 079a0de
fix: bound partial list element reads
mhk197 6ece0b5
remove bad pruning
mhk197 ef1d387
unchecked list construction
mhk197 c15e9fb
perf(layout): bound list reads by projection mask
mhk197 b98ccb0
perf(layout): zone lists before decomposition
mhk197 8a5fa6f
add element splits and documentation
mhk197 059de41
doc fix
mhk197 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,9 +50,11 @@ use vortex_layout::layouts::compressed::CompressingStrategy; | |
| use vortex_layout::layouts::compressed::CompressorPlugin; | ||
| use vortex_layout::layouts::dict::writer::DictStrategy; | ||
| use vortex_layout::layouts::flat::writer::FlatLayoutStrategy; | ||
| use vortex_layout::layouts::list::writer::ListLayoutStrategy; | ||
| use vortex_layout::layouts::repartition::RepartitionStrategy; | ||
| use vortex_layout::layouts::repartition::RepartitionWriterOptions; | ||
| use vortex_layout::layouts::table::TableStrategy; | ||
| use vortex_layout::layouts::table::use_experimental_list_layout; | ||
| use vortex_layout::layouts::zoned::writer::ZonedLayoutOptions; | ||
| use vortex_layout::layouts::zoned::writer::ZonedStrategy; | ||
| #[cfg(feature = "unstable_encodings")] | ||
|
|
@@ -157,6 +159,10 @@ pub struct WriteStrategyBuilder { | |
| allow_encodings: Option<HashSet<ArrayId>>, | ||
| flat_strategy: Option<Arc<dyn LayoutStrategy>>, | ||
| probe_compressor: Option<Arc<dyn CompressorPlugin>>, | ||
| /// Whether to write list fields using [`ListLayoutStrategy`]. | ||
| /// | ||
| /// [`ListLayoutStrategy`]: vortex_layout::layouts::list::writer::ListLayoutStrategy | ||
| use_list_layout: bool, | ||
| } | ||
|
|
||
| impl Default for WriteStrategyBuilder { | ||
|
|
@@ -170,6 +176,7 @@ impl Default for WriteStrategyBuilder { | |
| allow_encodings: Some(ALLOWED_ENCODINGS.clone()), | ||
| flat_strategy: None, | ||
| probe_compressor: None, | ||
| use_list_layout: use_experimental_list_layout(), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -184,6 +191,17 @@ impl WriteStrategyBuilder { | |
| self | ||
| } | ||
|
|
||
| /// Enable writing list fields with [`ListLayoutStrategy`]. | ||
| /// | ||
| /// **Note**: this is an unstable and experimental layout that is expected to change. | ||
| /// Using it may lead to unreadable files in the future. | ||
| /// | ||
| /// [`ListLayoutStrategy`]: vortex_layout::layouts::list::writer::ListLayoutStrategy | ||
| pub fn with_list_layout(mut self) -> Self { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment to this method? |
||
| self.use_list_layout = true; | ||
| self | ||
| } | ||
|
|
||
| /// Override the write layout for a specific field somewhere in the nested schema tree. | ||
| /// | ||
| /// The field path is matched after the root struct is split into columns. This is useful when a | ||
|
|
@@ -308,12 +326,14 @@ impl WriteStrategyBuilder { | |
| probe_compressor, | ||
| ); | ||
|
|
||
| let row_block_size = NonZeroUsize::new(self.row_block_size).vortex_expect("must be non 0"); | ||
|
|
||
| // 2. calculate stats for each row group | ||
| let stats = ZonedStrategy::new( | ||
| dict, | ||
| compress_then_flat.clone(), | ||
| ZonedLayoutOptions { | ||
| block_size: NonZeroUsize::new(self.row_block_size).vortex_expect("must be non 0"), | ||
| block_size: row_block_size, | ||
| ..Default::default() | ||
| }, | ||
| ); | ||
|
|
@@ -332,11 +352,37 @@ impl WriteStrategyBuilder { | |
| ); | ||
|
|
||
| // 0. start with splitting columns | ||
| let validity_strategy = CollectStrategy::new(compress_then_flat); | ||
| let validity_strategy = CollectStrategy::new(compress_then_flat.clone()); | ||
|
|
||
| // Take any field overrides from the builder and apply them to the final strategy. | ||
| let table_strategy = TableStrategy::new(Arc::new(validity_strategy), Arc::new(repartition)) | ||
| .with_field_writers(self.field_writers); | ||
| let mut table_strategy = | ||
| TableStrategy::new(Arc::new(validity_strategy), Arc::new(repartition)) | ||
| .with_field_writers(self.field_writers); | ||
|
|
||
| if self.use_list_layout { | ||
| // We need a closure here to enable recursive application of list layout. | ||
| table_strategy = table_strategy.with_list_layout_factory( | ||
| move |list_layout: ListLayoutStrategy| -> Arc<dyn LayoutStrategy> { | ||
| let zoned = ZonedStrategy::new( | ||
| list_layout, | ||
| compress_then_flat.clone(), | ||
| ZonedLayoutOptions { | ||
| block_size: row_block_size, | ||
| ..Default::default() | ||
| }, | ||
| ); | ||
| Arc::new(RepartitionStrategy::new( | ||
| zoned, | ||
| RepartitionWriterOptions { | ||
| block_size_minimum: 0, | ||
| block_len_multiple: row_block_size.get(), | ||
| block_size_target: None, | ||
| canonicalize: false, | ||
| }, | ||
| )) | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| Arc::new(table_strategy) | ||
| } | ||
|
|
||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // SPDX-FileCopyrightText: Copyright the Vortex contributors | ||
|
|
||
| use vortex_array::expr::Expression; | ||
| use vortex_array::expr::is_root; | ||
| use vortex_array::expr::not; | ||
| use vortex_array::expr::root; | ||
| use vortex_array::scalar_fn::fns::is_not_null::IsNotNull; | ||
| use vortex_array::scalar_fn::fns::is_null::IsNull; | ||
| use vortex_array::scalar_fn::fns::list_length::ListLength; | ||
| use vortex_error::VortexResult; | ||
|
|
||
| /// The minimal set of list children an expression needs for evaluation. | ||
| /// | ||
| /// For example: | ||
| /// - `is_null(root())` only needs the validity child. | ||
| /// - `list_length(root())` only needs the offsets and validity children. | ||
| /// - `root()` needs elements, offsets, and validity children. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub(super) enum ListChildrenNeeded { | ||
| /// Only the validity child is needed (`is_null` / `is_not_null`). | ||
| Validity, | ||
| /// Only the offsets and validity children are needed (`list_length`). | ||
| OffsetsAndValidity, | ||
| /// All children are needed. | ||
| All, | ||
| } | ||
|
|
||
| /// The minimal set of list children needed to evaluate `expr`, where `root()` is a field with list dtype. | ||
| pub(super) fn get_necessary_list_children(expr: &Expression) -> ListChildrenNeeded { | ||
| if is_null_root(expr) { | ||
| return ListChildrenNeeded::Validity; | ||
| } | ||
|
|
||
| if is_list_length_root(expr) { | ||
| return ListChildrenNeeded::OffsetsAndValidity; | ||
| } | ||
|
|
||
| if is_root(expr) { | ||
| return ListChildrenNeeded::All; | ||
| } | ||
|
|
||
| // Otherwise the requirement is the max over the operands. Childless expressions that never | ||
| // touch the list, such as literals, fall back to the cheapest usable child. | ||
| expr.children() | ||
| .iter() | ||
| .map(get_necessary_list_children) | ||
| .max() | ||
| .unwrap_or(ListChildrenNeeded::Validity) | ||
| } | ||
|
|
||
| fn is_null_root(expr: &Expression) -> bool { | ||
| (expr.is::<IsNull>() || expr.is::<IsNotNull>()) | ||
| && expr.children().len() == 1 | ||
| && is_root(expr.child(0)) | ||
| } | ||
|
|
||
| fn is_list_length_root(expr: &Expression) -> bool { | ||
| expr.is::<ListLength>() && expr.children().len() == 1 && is_root(expr.child(0)) | ||
| } | ||
|
|
||
| /// Rewrite a validity-class expression so it can be evaluated against the list's validity bool | ||
| /// array (`true` == valid row): `is_not_null(root())` becomes `root()` and `is_null(root())` | ||
| /// becomes `not(root())`. All other nodes are rebuilt with rewritten children. | ||
| pub(super) fn rewrite_validity_expr(expr: &Expression) -> VortexResult<Expression> { | ||
| if expr.is::<IsNotNull>() && expr.children().len() == 1 && is_root(expr.child(0)) { | ||
| return Ok(root()); | ||
| } | ||
| if expr.is::<IsNull>() && expr.children().len() == 1 && is_root(expr.child(0)) { | ||
| return Ok(not(root())); | ||
| } | ||
| let children = expr | ||
| .children() | ||
| .iter() | ||
| .map(rewrite_validity_expr) | ||
| .collect::<VortexResult<Vec<_>>>()?; | ||
| expr.clone().with_children(children) | ||
| } | ||
|
|
||
| /// Rewrite an offsets-class expression so it can be evaluated against an array of list lengths. | ||
| /// `list_length(root())` becomes `root()`. Other references to `root()` are left intact: for | ||
| /// offsets-class expressions they can only be validity checks, and the lengths array carries the | ||
| /// same validity as the original list. | ||
| pub(super) fn rewrite_offsets_expr(expr: &Expression) -> VortexResult<Expression> { | ||
| if is_list_length_root(expr) { | ||
| return Ok(root()); | ||
| } | ||
|
|
||
| let children = expr | ||
| .children() | ||
| .iter() | ||
| .map(rewrite_offsets_expr) | ||
| .collect::<VortexResult<Vec<_>>>()?; | ||
| expr.clone().with_children(children) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use rstest::rstest; | ||
| use vortex_array::dtype::DType; | ||
| use vortex_array::dtype::Nullability; | ||
| use vortex_array::dtype::PType; | ||
| use vortex_array::expr::cast; | ||
| use vortex_array::expr::eq; | ||
| use vortex_array::expr::gt; | ||
| use vortex_array::expr::is_not_null; | ||
| use vortex_array::expr::is_null; | ||
| use vortex_array::expr::list_length; | ||
| use vortex_array::expr::lit; | ||
| use vortex_array::expr::not; | ||
| use vortex_array::expr::root; | ||
|
|
||
| use super::*; | ||
|
|
||
| /// `get_necessary_list_children` keys off the deepest list child an expression touches; `All` | ||
| /// is the always-correct default for anything not specifically recognized. | ||
| #[rstest] | ||
| // `is_null` / `is_not_null` of the list itself need only validity. | ||
| #[case::is_null(is_null(root()), ListChildrenNeeded::Validity)] | ||
| #[case::is_not_null(is_not_null(root()), ListChildrenNeeded::Validity)] | ||
| // Compound over validity-only operands stays validity. | ||
| #[case::not_is_null(not(is_null(root())), ListChildrenNeeded::Validity)] | ||
| // A list-independent (constant) expression falls to the cheapest usable child. | ||
| #[case::constant(lit(5), ListChildrenNeeded::Validity)] | ||
| // `list_length(root())` needs offsets and validity, but not elements. | ||
| #[case::list_length(list_length(root()), ListChildrenNeeded::OffsetsAndValidity)] | ||
| // Compound over offsets-only operands stays offsets. | ||
| #[case::list_length_filter( | ||
| gt(list_length(root()), lit(1u64)), | ||
| ListChildrenNeeded::OffsetsAndValidity | ||
| )] | ||
| #[case::cast_list_length( | ||
| cast( | ||
| list_length(root()), | ||
| DType::Primitive(PType::I64, Nullability::Nullable), | ||
| ), | ||
| ListChildrenNeeded::OffsetsAndValidity | ||
| )] | ||
| // A bare list reference needs the elements. | ||
| #[case::bare_root(root(), ListChildrenNeeded::All)] | ||
| // Any other fn over the list needs the elements. | ||
| #[case::not_root(not(root()), ListChildrenNeeded::All)] | ||
| // `is_null` only short-circuits to validity when its argument is the list itself. | ||
| #[case::is_null_of_derived(is_null(not(root())), ListChildrenNeeded::All)] | ||
| // Max over operands: validity + elements => elements. | ||
| #[case::validity_and_elements(eq(is_null(root()), root()), ListChildrenNeeded::All)] | ||
| fn classify_expr_class(#[case] expr: Expression, #[case] expected: ListChildrenNeeded) { | ||
| assert_eq!(get_necessary_list_children(&expr), expected); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment that this should only be enabled for experiments right now, will change and will lead to broken files in the future