Update columnar 0.12, timely 0.28, differential-dataflow 0.21#35674
Update columnar 0.12, timely 0.28, differential-dataflow 0.21#35674antiguru merged 2 commits intoMaterializeInc:mainfrom
Conversation
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
def-
left a comment
There was a problem hiding this comment.
The nightly performance regressions are concerning, but you've probably seen those: https://buildkite.com/materialize/nightly/builds/15871
e12a447 to
626ab54
Compare
def-
left a comment
There was a problem hiding this comment.
No concerns from testing side!
| use differential_dataflow::lattice::Lattice; | ||
| use differential_dataflow::operators::arrange::{Arranged, TraceAgent}; | ||
| use differential_dataflow::trace::implementations::merge_batcher::container::MergerChunk; | ||
| use differential_dataflow::trace::implementations::merge_batcher::container::InternalMerge; |
There was a problem hiding this comment.
I hope to delete this in the next DD. Should check, but I think it is a near-vestigial use for reduce just to access .clear().
There was a problem hiding this comment.
I have a follow-up to vendor everything around columnation/TimelyStack into Mz so we can remove all of it from DD.
| logger: Option<differential_dataflow::logging::Logger>, | ||
| operator_id: usize, | ||
| inner: MergeBatcher<Vec<(D, T, R)>, ColumnationChunker<(D, T, R)>, ColMerger<D, T, R>>, | ||
| inner: MergeBatcher<Vec<(D, T, R)>, ColumnationChunker<(D, T, R)>, ColInternalMerger<D, T, R>>, |
There was a problem hiding this comment.
I'm not sure what this is through the aliases, but we should double check async. Some of the mergers are just bad, and some of them are better than others.
src/compute/src/logging/timely.rs
Outdated
| // SAFETY: `datum.typ` originates from a Rust `String` field | ||
| // (`ChannelsEvent::typ`) serialized through columnar, which | ||
| // now returns `&[u8]` instead of `&str` to skip validation. | ||
| Datum::String(unsafe { std::str::from_utf8_unchecked(datum.typ) }), |
There was a problem hiding this comment.
This is a behavioral change where we used to panic, but will now be UB. Worth double-checking which we want (personally prefer the panic potential).
src/expr/src/lib.rs
Outdated
| //! Core expression language. | ||
|
|
||
| #![warn(missing_debug_implementations)] | ||
| #![recursion_limit = "256"] |
There was a problem hiding this comment.
If you can leave a note about why, that would be helpful for the future.
frankmcsherry
left a comment
There was a problem hiding this comment.
Looked at everything other than Cargo.lock.
- The PR doesn't mention, but there are a substantial number of
Columnarderivations added. I think this is good, personally, but it locks in some obligations (e.g. if folks rely on them, they can no longer be removed). - There is new
unsafein the codebase that I think we should avoid. If we find utf8 validation in logging dataflows is a hotspot we can add it in, but imo let's default to safe.
|
Thanks for the review. I backed out the |
Updates * columnar to 0.12.0 * timely to 0.28.0 * differential-dataflow to 0.21.0 Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
…hanges Update our code to match the breaking API changes in the bumped dependencies: **columnar 0.12:** - `EncodeDecode` trait and `Indexed` struct removed; replaced by `columnar::bytes::indexed` module with free functions - `HeapSize` trait removed; delete all implementations - `FromBytes` now requires `const SLICE_COUNT: usize` - `Strings::Ref` is now `&[u8]` instead of `&str`; use `from_utf8_unchecked` where the data is known-valid UTF-8 **timely 0.28:** - `Operate` trait: `get_internal_summary()` + `set_external_summary()` replaced by `initialize(self: Box<Self>)` which consumes the operator and returns `(Connectivity, SharedProgress, Box<dyn Schedule>)` - `notify_me()` returns `&[FrontierInterest]` instead of `bool` - `set_notify(false)` replaced by per-input `set_notify_for(i, FrontierInterest::Never)` - `MutableAntichain::new_bottom` renamed to `from_elem` **differential-dataflow 0.21:** - `ColMerger` renamed to `ColInternalMerger` (in `container` submodule) - `MergerChunk` trait renamed to `InternalMerge` - `ColInternalMerger` now requires `D: Clone` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>
Summary
columnar 0.12
EncodeDecodetrait andIndexedstruct replaced bycolumnar::bytes::indexedmoduleHeapSizetrait removedFromBytesnow requiresconst SLICE_COUNT: usizeStrings::Refchanged from&strto&[u8]timely 0.28
Operatetrait redesigned:get_internal_summary()/set_external_summary()replaced byinitialize(self: Box<Self>)which returns(Connectivity, SharedProgress, Box<dyn Schedule>)notify_me()returns&[FrontierInterest]instead ofboolset_notify(false)replaced by per-inputset_notify_for(i, FrontierInterest::Never)MutableAntichain::new_bottomrenamed tofrom_elemdifferential-dataflow 0.21
ColMergerrenamed toColInternalMergerMergerChunktrait renamed toInternalMergeTest plan
cargo checkpassescargo clippycleancargo fmtclean🤖 Generated with Claude Code