refactor(line): replace the nth kernel and grouped fold with 1 gather expr - #46
Merged
Conversation
…e gather expression `nth` downsampling is now a single Polars `gather` (`_nth_agg_expr`) whose positions come from the column's own `len()`, so the same expression serves the resident select, the grouped `group_by` on both source kinds, and the geo line. It returns exactly what the Rust kernel returned: stride `max(1, len // n_points)`, at most `n_points` rows, every row when short. Removed: - the `every_nth` Rust kernel, its Python binding, namespace method and tests - `grouped_nth_plan`, the `collect_batches` fold and the `_NTH_*` constants - `_geo_line_nth_agg_expr` and geo_line's plugin import Kept `nth_plan` for the ungrouped scan: its two streaming passes stay flat in rows and its frame-level viewport filter prunes row groups, which the expression cannot do. A grouped `nth` on a scan now gathers in memory, because Polars 1.44 streams no non-reduction aggregation. `test_ooc.py` marks `line-grouped-nth` a strict xfail. Zoomed, its memory follows the visible window and it is 2.7-3.4x faster than the fold; unzoomed it is O(rows), which was accepted. The fold was also the site of two Polars hazards found on 16- and 32-thread machines: the streaming engine runs ahead of a slow batch consumer by about 25 MB per thread, and a sparse `filter` at low thread counts can return a zero-copy view that pins whole batches. BREAKING CHANGE: `pl.Expr.flexviz.every_nth` no longer exists.
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
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.
nthdownsampling is now a single Polarsgather(_nth_agg_expr) whose positions come from the column's ownlen(), so the same expression serves the resident select, the groupedgroup_byon both source kinds, and the geo line. It returns exactly what the Rust kernel returned: stridemax(1, len // n_points), at mostn_pointsrows, every row when short.Removed:
every_nthRust kernel, its Python binding, namespace method and testsgrouped_nth_plan, thecollect_batchesfold and the_NTH_*constants_geo_line_nth_agg_exprand geo_line's plugin importKept
nth_planfor the ungrouped scan: its two streaming passes stay flat in rows and its frame-level viewport filter prunes row groups, which the expression cannot do.A grouped
nthon a scan now gathers in memory, because Polars 1.44 streams no non-reduction aggregation.test_ooc.pymarksline-grouped-ntha strict xfail. Zoomed, its memory follows the visible window and it is 2.7-3.4x faster than the fold; unzoomed it is O(rows), which was accepted.The fold was also the site of two Polars hazards found on 16- and 32-thread machines: the streaming engine runs ahead of a slow batch consumer by about 25 MB per thread, and a sparse
filterat low thread counts can return a zero-copy view that pins whole batches.BREAKING CHANGE:
pl.Expr.flexviz.every_nthno longer exists.