Repository navigation
Release 2.0.0: sum, zip_binary, cum_agg_runs, Array support, dtype preservation, and the strict/error-policy overhaul - #8
Merged
Conversation
…ing the process Without the feature, polars-core's arrow-FFI import has no FixedSizeList arm compiled in and hits a non-unwinding panic before any plugin entry check runs, taking the whole host process down (exit 0xC0000409, no Python traceback). With it, the plugin's own dtype check rejects Array input with a clean ComputeError naming the received dtype. Array support proper comes later; this is the independent bug fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t checks Implements PR 1 of the spec (List columns only): - "sum" joins the aggregation vocabulary of agg_slices and agg_lists, with polars' identity element for an empty selection (+0.0). The Aggregation enum moves to statistic.rs, shared with the new scan. - zip_binary applies a binary op element-wise between two list columns, deferring per element to the corresponding scalar polars op: null propagation and IEEE arithmetic, Kleene logic for and/or, and polars' total order for comparisons (NaN equals NaN, exceeds everything). - cum_agg_runs cumulatively aggregates within gate-delimited runs. Each emitted position equals the vertical aggregation over the run prefix, so an all-True gate reproduces cum_sum/cum_min/cum_max/cum_count, including cum_count's emission at null value positions. - strict (default off) adds the checks whose violations have a legitimate degenerate reading: apply_interp monotonicity, agg_slices range validity, and agg_lists overlength rows. The last rides on a pass-through plugin because pure-polars expressions cannot raise. Both new plugins ship their final dtype contract: Float32 preserved, count as UInt32, integer inners rejected. They also treat empty lists as valid (empty in, empty out), being total over zero elements. Tests add a scan oracle driven by polars itself, deference anchors against native cum_*, strict positive/negative pairs, and a plan-purity test asserting the streaming engine keeps every plugin node in-stream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m review Adversarial review of the PR 1 draft, against polars as the anchor. Numerics, all verified against polars over adversarial value matrices: - The even-count median used (a + b) / 2, which overflows for large equal values and loses precision generally: the median of two 1e308s read inf, and [1.0, -inf] read -inf where polars gives NaN. polars interpolates instead, a + (b - a) * 0.5, short-circuiting on equality so that equal infinities do not read NaN. The interpolated form matches polars on every probed edge case and on 30000/30000 random pairs, where the midpoint form misses 8679. - min/max broke signed-zero ties the wrong way. polars keeps the first of values that compare equal; f64::min/max keep the second, so [-0.0, 0.0] and [0.0, -0.0] both came out sign-flipped. The scan's median insert had the same bug in another guise, using a lower bound where a stable sort needs an upper one. - sum folded from IEEE's -0.0 identity, so an all-negative-zero selection read -0.0 where polars reads +0.0. - The scan's Welford variance reported zero spread for values that differ, whenever the running mean rounded onto the next value -- at timestamp magnitudes an ordinary input, and the worst possible failure for a std feeding a threshold. std and median now delegate to the same kernels agg_slices uses, so one input gives one answer everywhere in the library, at the cost of a retained buffer for those two aggregations. Kwargs cross the plugin boundary as a pickle, and the decoder cannot resolve a memoized container back-reference. Any range object, bound tuple or range list appearing twice therefore killed the query with "recursive structure found" -- and CPython's constant folding arranges exactly that for a literal as ordinary as a single-point closed range. The wrapper now rebuilds ranges into fresh plain-float tuples, which also keeps numpy scalars off the wire, and accepts the list-shaped form that strict validation previously rejected with a TypeError. Errors raised inside a row named a row index. Plugins are handed one chunk at a time, so that index was chunk-local: it changed with chunk layout and with the engine, and pointed at a different, valid row. The messages now describe the offending row by its shape instead. apply_interp's strict check no longer treats NaN as a violation. The spec puts NaN on the propagating side of the family rule, and the check exists for unsorted input, which fails silently; a NaN announces itself in the output. Docs now state what the code does: Float32 accumulates in f64 and rounds once, so it can diverge from polars' per-step rounding; the native cum_min/cum_max seed leak affects leading infinities as well as NaN; and agreement with polars is semantic, not bit-for-bit, since accumulation order differs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two conventions exist for the sum of no values. Polars answers 0.0, the identity element of addition, which is what makes summation decompose. For feature extraction that is the dangerous answer: an empty selection usually means a misconfigured range, and a real 0.0 disguises a missing measurement as a quiet one, which then sails through a threshold. Rather than pick one and bury it, agg_slices and agg_lists now take empty="null" (default) or empty="zero", so the choice is visible at the call site and either camp is one argument away. It reaches exactly one cell of the matrix: sum is the only aggregation with a non-null identity, every other one yields null over nothing either way, and count yields 0 either way. cum_agg_runs needs no such flag, since polars' own cum_sum already emits null there and both camps agree. The flag is deliberately not folded into strict, which means one thing -- turn a silent degenerate reading into an error -- and never changes a returned value. Enabling it to catch unsorted x should not silently move your feature values. strict itself now defaults to True, for the same reason the empty default is null: every check it governs covers a failure that is otherwise invisible, and this library's job is feature extraction, not faithful reproduction of numpy's silence. strict=False keeps every lenient reading, and the explode-equivalence carve-out for sum is now written into the suite explicitly rather than left implied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The new docs overclaimed and two of the strict checks were miscalibrated. empty="zero" was documented as equivalent to .fill_null(0.0). It is not, and the difference runs in the unsafe direction: agg_slices yields null both for an empty selection, which the flag governs, and for a null row, which it deliberately does not. fill_null cannot tell the two apart, so following that advice launders a missing measurement into a real zero -- the exact failure the flag exists to prevent. The claim of agreement with explode/group_by is also formulation-dependent, so it now names the formulation, and the test that carried the name performs the round trip instead of asserting 0.0 == 0.0. An empty list row is a null row today rather than an empty selection, so neither empty nor count's zero reaches it. The spec changes that in the breaking pass; until then the docs say so and a test pins it, so the divergence from agg_lists is deliberate rather than discovered. agg_lists selected its aggregation table by testing empty against the string "null" and falling through otherwise, so any typo, or the None a JSON null decodes to, silently chose the zero convention -- picking the dangerous reading precisely when the caller misspelled the safe one. Both aggregating functions now validate aggregation and empty at expression construction and fail the same way. apply_interp's monotonicity check compared adjacent pairs, and every IEEE comparison with NaN is false, so a NaN between two elements hid the descent: [0, 10, NaN, 1, 2] passed and interpolated finite, plausible, wrong numbers. It now compares each coordinate against the last non-NaN one, so NaN stays legal and propagates while the descent around it raises. agg_lists' strict check raised on any row longer than list_length, but truncating a null-padded tail discards nothing, since every aggregation skips nulls. Fixed-capacity buffers and widened ragged data look like that, and the only remedy was disabling the check for the rows that do lose data. It now raises only when something past the window is non-null. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The spec is a local working document, not a shipped artefact, so it is untracked and ignored rather than committed alongside the code. The version goes straight to 2.0.0: everything on this branch lands as a single breaking release, so there is no intermediate number to occupy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kernels only ever see List. An Array is normalized on the way in and rebuilt on the way out, so container handling lives entirely at the entry/exit boundary and every function keeps one kernel. polars casts between the two losslessly -- inner dtype, null rows, inner nulls all survive -- which is what makes the round trip free. The container algebra: length-preserving functions return what they were given; apply_fft narrows Array(w) to Array(w/2 + 1), being the only width-transforming function; apply_interp follows xp, since the query coordinates decide the output's shape. zip_binary and cum_agg_runs return an Array only when both inputs are Arrays, and their widths must agree -- a mismatch the schema already knows about, so it raises before execution rather than per row. A mixed List/Array pair is allowed and gives a List. Two functions sit outside that. agg_slices reduces to a scalar, so it has no container to propagate. agg_lists always returns a List: it reduces many rows to one, which an elementwise plugin cannot do, so it must stay an expression composition -- and an expression is built before any schema is resolved, so the container it would have to choose is not knowable there. It gains a pass-through plugin to normalize Array to List, since the .list namespace rejects Array outright; that plugin also absorbed the strict width check and now validates list_length against an Array's width. Zero-width Arrays are rejected rather than supported. polars-arrow panics when slicing a zero-width FixedSizeList, inside the FFI boundary before any of this code runs, so the case is not ours to support. Every function resolves its output type at plan time, which is what turns that panic into a typed error -- agg_slices now resolves its Float64 through a function rather than declaring it statically, purely so the check runs. Zero-length Lists are unaffected. The three tests that pinned "Array raises" become support tests. The original PR 0 regression is kept honest by asserting that the calls which once aborted the process now complete. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adversarial review of the Array pass found three ways to reach a polars-arrow panic that the plan-time guard was meant to have closed. apply_interp resolved its output from xp alone, so nothing inspected x or y. A zero-width Array in either of those positions sailed past the guard and panicked, while the identical column in the xp slot raised cleanly -- the failure mode depended purely on argument position. Every function must resolve every container input, not only the one that governs its output shape, and the tests now cover all eleven positions rather than three functions. The guard also matched only the outermost dimension, so Array(Float64, (2, 0)) -- which is Array(Array(Float64, 0), 2), outer width an innocent 2 -- walked straight through to the same panic. It now rejects a zero width at any nesting depth. The third route was not about Arrays at all. polars gates the narrow integer dtypes behind cargo features, and reconstructs the Series across the FFI boundary before any plugin code runs, so List(Int8) panicked there exactly as Array once did -- and no plan-time check can help, because the panic precedes our code. The features are now enabled, which is the same one-line remedy the Array abort took. Two structural checks the spec requires were also missing: apply_interp's x/y pair and agg_slices' value/index pair are decidable from the schema when both sides are Arrays, but only zip_binary and cum_agg_runs were enforcing theirs. All four now share one helper, so the asymmetry cannot come back quietly. Docs corrected where the Array pass had outdated them: agg_lists no longer claims to be pure polars, and now states that an Array input must match list_length exactly rather than being null-padded like a short List. The mixed-container rules are documented as the deliberate asymmetry they are -- zip_binary needs both operands to be Arrays, while cum_agg_runs follows its value column, its gate being a mask rather than an operand. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Probing the whole dtype space turned up five more inners that panic rather than error: Categorical, Enum, Decimal, Struct and Int128. The cause is the one PR 0 identified for Array -- polars reconstructs the Series across the plugin's arrow-FFI boundary before any of our code runs, so a dtype whose cargo feature is absent panics somewhere no check of ours can reach. Hand-picking the four narrow integer features fixed the case the review reported and left the other five. dtype-full fixes the class, and stops the set growing silently the next time polars gates a new dtype. Rejecting a dtype is this library's job; being able to see it is polars'. The wheel grows to about 25 MB, which is the price of the doctrine that plugins never panic. Int128 turns out to be accepted rather than rejected once visible, being primitive and numeric -- so the test asserts the actual invariant, that no dtype produces a panic, rather than presuming which ones we reject. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The last piece of the 2.0 contract: the library stops converting invalid data into missing data, and stops flattening every dtype to Float64. Dtypes. Float32 in gives Float32 out for all seven functions now, not just the two that shipped with it: kernels compute in f64 and round once at the output boundary, so Float32 results are deterministic rounded-once values. apply_interp's output follows its y column, values determining the value dtype. count emits UInt32 -- polars' count dtype -- from agg_slices and agg_lists as it already did from cum_agg_runs, the one exception to preservation. Integer inners raise at plan time naming the parameter, instead of being silently cast; integer support would multiply the vetting matrix and is deliberately out of scope. Error policy. Wrong shape is structural, not data, so it raises where it used to yield a null row: a non-power-of-two FFT length (or one below 2, where the windows sum to zero), a Butterworth signal shorter than its reflection padding, and an empty list into any transform -- the limiting case of too-short, and a raise policy that softened as the input got worse would be indefensible. For an Array the width sits in the schema and these raise at plan time; for mixed-length List data the escape hatch is pre-filtering with list.len(). Non-finite samples go the other way: they are legitimate floats that now propagate -- one NaN yields an all-NaN spectrum instead of a null row, because converting invalid to missing erased exactly the distinction the null/NaN protocol depends on. Internal kernel failures raise rather than nulling, and butterworth validates sample_rate itself so the message names the parameter. Empty lists split by family. Transforms raise, as above. Aggregations treat them as the empty selection they are -- count 0, sum per the empty flag -- which closes the one disagreement between agg_slices and agg_lists. apply_interp splits precisely: an empty x/y is no interpolant and raises, an empty xp is zero query points and returns an empty list. The FFT scaling vocabulary becomes scipy's: power -> spectrum, psd -> density, with the old names removed rather than aliased -- the ambiguous name disappearing is the point, "power" having already been misread as A-squared in practice. amplitude_squared is added as exactly that quantity, the square of the amplitude scaling with the one-sided doubling squared along with it. Window names are untouched. Tests pinned to the old behavior are flipped deliberately per the plan, and a dtype-preservation suite covers the new contract per function, aggregation and container. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One missed sibling edit and five smaller findings from the adversarial review of the dtype and error-policy pass. The README's empty-flag section still described the pre-2.0 rule that a row with empty lists stays null -- three paragraphs after the same file correctly said an empty list is now an empty selection, and one file away from the docstring this pass had already corrected. The paragraph now matches both. apply_interp's closing Returns line likewise still promised List[f64], contradicting the dtype rule two paragraphs above it in its own docstring. Butterworth parameter errors now all carry the function name and the offending keyword: the cutoff range is checked in the plugin rather than left to the butterworth crate, whose errors name neither and leak a transitive dependency's Debug enum into a user-facing message. A NaN cutoff fails the range check and raises too. min_samples saturates instead of wrapping, so an absurd filter_order can no longer arithmetically defeat the plan-time Array guard in a release build, and filter_order >= 1 is validated at plan time alongside sample_rate. The escape-hatch docs now say the list.len() pre-filter must sit upstream of the transform: in a lazy query the optimizer pushes a later filter below the plugin, so offending rows are dropped before validation rather than raising, and the eager and lazy outcomes of the post-filter form differ. A streaming variant of the pre-filter test pins the direction that works. Two coverage holes: the amplitude_squared test ran on a zero-mean tone whose DC and Nyquist bins sit at 1e-15, where the default absolute tolerance would accept a wrong one-sided factor at exactly the two bins the doubling skips -- the signal now carries a DC offset and the two bins are asserted explicitly, along with the documented spectrum-vs-amplitude-squared wrinkle at DC. And the rename test only probed "power": "psd" must be rejected too, and now is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The house pattern for collect(engine=...), whose stub returns InProcessQuery | DataFrame. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The whole-release cross-check found the ground shifting under the suite: polars fixed the very bugs this library had documented itself as deviating from. On >= 1.40 the cum_min/cum_max fold-seed leak is gone (1.44 agrees exactly with this library's prefix rule), 1.44 flipped polars' own min/max tie-break from first-wins to last-wins, and 1.44 widened the Float32 cum_sum accumulator. Eight tests asserted polars' old behavior via live oracles, so CI -- which installs the newest polars, not the lock -- would have failed its gating job and a 2.0.0 tag would have published nothing. The library's semantics do not move: the prefix rule, first-wins ties, and round-once Float32 were always version-independent choices, and they stay. The tests now assert that contract directly -- the oracle computes min/max with its own first-wins fold instead of leaning on a polars whose tie-break drifted, and the two remaining native comparisons branch on the polars version. The suite passes 304/304 on both 1.30 (the lock) and 1.44.1 (what CI installs). The docs that described the deviations as facts about polars now scope them to the versions where they were true. The dev extra records that the tests need polars >= 1.25 for the streaming engine, though the library itself still runs on the declared 1.23 floor. Behavioral fixes from the same review: Statistic::mean now routes through the +0.0-seeded sum, so an all-negative-zero mean reads +0.0 everywhere (agg_slices had kept -0.0 while cum_agg_runs and polars said +0.0); broadcast-length errors carry the calling function's name instead of being the one anonymous message reachable from six plugins; and a typo'd range boundary mode fails at construction naming the parameter instead of surfacing serde's untagged-enum internals. Coverage the review showed to be missing or toothless, now present: even-count median VALUES for agg_slices against polars at the extremes; the kwargs rebuild exercised on slices_exclude alone (a shared object in both kwargs stops aliasing once include is rebuilt, so those tests could not catch a missing exclude rebuild); a +-inf FFT row (a mix of inf and NaN bins, not all-NaN -- the spec bullet said otherwise and now matches the docs); null elements pinned across all six transform entry paths including the butterworth no-cutoff pass-through; agg_lists Float32 preservation; and an agg_slices f32 fixture built around 2^24, where step-wise f32 accumulation and f64-rounded-once actually part company. Docs: README gains the requested authorship disclaimer and a 2.0.0 changelog section; the "six plugin functions" broadcast paragraph is rewritten to say what actually broadcasts and why the count was misleading; the agg_lists bullet no longer claims "pure Polars (no plugin)" and now states that it aggregates in the column's own dtype (overflowing near the Float32 limit where the round-once kernels stay finite); the strict bullet no longer overstates the agg_lists check; and the inverted +-/-+ signs in the seed-leak note are replaced by explicit per-function values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
They stay untracked and uncommitted, and git status simply shows them -- preferred over a permanent ignore entry for files that will move elsewhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A LICENSE file plus the pyproject and Cargo metadata, so PyPI stops showing the package as all-rights-reserved and cargo stops warning about a missing license during the sdist build. The wheel metadata carries License-File plus the MIT classifier; the license file ships in every wheel and the sdist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
python -c prepends its working directory to sys.path, and at the repo root the source directory shadows the installed package. Locally the shadow is harmless -- maturin develop drops the compiled module into the source tree -- but on CI, where the package is pip-installed into site-packages, the shadow has no _internal module and the import dies. The subprocess now runs from the tests directory, which cannot shadow anything. Only this test was affected: it is the suite's one subprocess, and pytest itself puts tests/ on the path, not the root. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
One bundled breaking release — everything since
release-1.0.0, developed and reviewedon this branch, squash-merged as a single commit and tagged
release-2.0.0.What's new
sumaggregation everywhere, with anemptyswitch deciding what the sum ofnothing is:
"null"(default — missing, not zero) or Polars'"zero".zip_binary: element-wise arithmetic, Kleeneand/or, and total-ordered comparisonsbetween two list columns.
cum_agg_runs: cumulative aggregation within gate-delimited runs, matching Polars'cum_*semantics including thecum_countnull-position exception.Arraycolumns accepted by every function, with the container preserved(
apply_fftmapsArray(w)→Array(w//2 + 1)).Breaking
strictchecks default on: unsorted interpolation axes, invalid ranges, and lossyagg_liststruncation raise (strict=Falserestores the old leniency).too-short Butterworth signals, empty lists into transforms. Escape hatch:
pre-filter with
list.len()upstream of the transform.thresholds with
is_finite().Float32in →Float32out (computed in f64, rounded once);count→UInt32;integer inner dtypes raise.
power→spectrum,psd→density,plus new
amplitude_squared. Window names unchanged (hanningstays).mediannow matches Polars exactly (last-ulp changes on stored features).Fixed
Arrayinput no longer aborts the host process; no input dtype can panic(polars built with
dtype-full— the wheel grows to ~25 MB).agg_slices.-0.0-identity corrections inmin/max/sum/mean.Review notes
The 16 commits read as a narrative in order: features → review fixes → design decisions
→ Array pass → panic hunting → semantics pass → modern-polars survival. The decisions
most worth scrutiny (mine/ours rather than the spec's):
strict=Trueandempty="null"defaults,
dtype-fullvs. wheel size, first-wins signed-zero ties as a fixed rule whilePolars 1.44 changed its own, and the version-scoped deviation notes.
Verified: 304 tests green on polars 1.30.0 and 1.44.1 (CI installs newest), clippy
-D warnings, rustfmt, ruff, ty, showcase example. Four adversarial review rounds ranagainst this branch; all confirmed findings are fixed in the history.
Merge as squash, using the prepared squash message (title + summary + all 16 commit
subjects), then tag the squash commit
release-2.0.0— CI publishes to PyPI from the tag.🤖 Generated with Claude Code