Migrate to new stats falsification rules by gatesn · Pull Request #8345 · vortex-data/vortex

gatesn · 2026-06-10T22:12:20Z

Port file pruning to session stats rewrites

## Summary Adds docs for `StatsRewriteRule` and its functions. Can be considered a follow-up for #8345, but can be individually merged. Signed-off-by: Adam Gutglick <adam@spiraldb.com>

Port file pruning to session stats rewrites Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

codspeed-hq · 2026-06-11T15:52:09Z

Merging this PR will improve performance by 13.18%

⚠️

Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 7 improved benchmarks
❌ 2 regressed benchmarks
✅ 1527 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`decompress_rd[f64, (100000, 0.0)]`	845.6 µs	1,024.6 µs	-17.47%
❌	Simulation	`decompress_rd[f32, (100000, 0.0)]`	499.2 µs	587 µs	-14.96%
⚡	Simulation	`chunked_bool_canonical_into[(1000, 10)]`	35.5 µs	20.4 µs	+73.82%
⚡	Simulation	`chunked_varbinview_canonical_into[(1000, 10)]`	198.1 µs	161.6 µs	+22.57%
⚡	Simulation	`chunked_varbinview_into_canonical[(1000, 10)]`	213.5 µs	177 µs	+20.62%
⚡	Simulation	`decompress_rd[f64, (100000, 0.01)]`	981.4 µs	846 µs	+16%
⚡	Simulation	`decompress_rd[f64, (100000, 0.1)]`	981.3 µs	846 µs	+16%
⚡	Simulation	`bitwise_not_vortex_buffer_mut[128]`	244.4 ns	215.3 ns	+13.55%
⚡	Simulation	`bitwise_not_vortex_buffer_mut[1024]`	304.7 ns	275.6 ns	+10.58%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing ngates/public-stats-rewrite-rules (ffbccee) with develop (8acef3a)}

Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

Signed-off-by: Nicholas Gates <nick@nickgates.com>

AdamGS · 2026-06-12T19:09:06Z

        with:
          sccache: s3
      - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10  # v6
+      - uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654  # v5


AdamGS · 2026-06-12T19:11:12Z

+use crate::scalar::Scalar;
+use crate::scalar_fn::fns::stat::StatFn;
+
+/// A target that can bind abstract statistics to concrete expressions.


What does it mean to bind a statistic to an expression? What makes statistics abstract?

What does it mean to bind a statistic to an expression? What makes statistics abstract?

Yeah stats stuff I am intermingling with an expressions change, so I think the "stat" expression becomes explicitly an "Expression::Placeholder", meaning it must be replaced prior to execution.

It's the same logic as our StatCatalog. Except the current StatCatalog means you have to re-run falsification over the entire expression any time your stats come from a different place, e.g. FileStats vs ZoneMap vs ArrayStats.

Here you take the falsified expression, then "bind" the stats from wherever you get them from.

AdamGS · 2026-06-12T19:30:01Z


+    fn lower_stats(&self, predicate: Expression) -> VortexResult<Expression> {
+        let mut binder = FileStatsBinder { reader: self };
+        let Some(predicate) = bind_stats(predicate, &mut binder)? else {


Looking at bind_stats not sure I understand how the error message here is related

AdamGS · 2026-06-12T19:33:05Z

+        return Some(FieldPath::root());
+    }
+
+    let field_name = expr.as_opt::<GetItem>()?;


unrelated to this PR - but this is a really confusing API

AdamGS · 2026-06-12T19:35:31Z

+    binder.bind_aggregate(input, aggregate_fn, &stat_dtype)
+}
+
+fn null_expr(dtype: DType) -> Expression {


nit - just inline? I feel like these short utility functions always make the code much harder to trace

AdamGS · 2026-06-12T19:37:06Z

+) -> VortexResult<Option<Expression>> {
+    let options = expr.as_::<StatFn>();
+    let aggregate_fn = options.aggregate_fn();
+    let input = expr.child(0);


Why is this always 0?

AdamGS · 2026-06-12T19:38:14Z

+        .into_inner();
+
+    #[expect(deprecated)]
+    let lowered = lowered.simplify_untyped()?;


can't you do lowered.optimize_recursive(&scope)?

AdamGS · 2026-06-12T19:53:22Z

@@ -115,14 +119,52 @@ impl FileStatsLayoutReader {
        Ok(result.as_bool().value() == Some(true))


nit - unwrap_or_default()

AdamGS · 2026-06-12T19:55:56Z

+    refs
+}
+
+fn collect_referenced_stat_field_names(expr: &Expression, refs: &mut HashSet<FieldName>) {


we have Node implemented for Expression, you should be able to visit the tree using that instead of hand-rolling the recursion

AdamGS · 2026-06-12T19:56:19Z

+    }
+}
+
+fn bool_literal(expr: &Expression) -> Option<Option<bool>> {


called once, inline

AdamGS · 2026-06-12T19:57:37Z

+        return Ok(None);
+    };
+    let required_stats = filter_required_stats(&lowered, binder.required_stats);
+    if required_stats.map().is_empty() && !matches!(bool_literal(&lowered), Some(Some(true))) {


maybe add a comment explaining this if statement?

AdamGS · 2026-06-12T19:59:47Z

        available_stats,
+        required_stats: Relation::new(),
    };
+    let Some(lowered) = bind_stats(predicate, &mut binder)? else {


Maybe I'm just stupid - but I think both "lowering" and "bindings" are worth defining and explaining somewhere.

AdamGS · 2026-06-12T20:00:41Z

I think I mostly have a lot of questions 😅

gatesn added the changelog/chore A trivial change label Jun 10, 2026

AdamGS mentioned this pull request Jun 11, 2026

Add docs for StatsRewriteRule #8355

Merged

gatesn force-pushed the ngates/public-stats-rewrite-rules branch from 033c73f to 5ed375f Compare June 11, 2026 15:49

gatesn added 2 commits June 11, 2026 11:51

Make stats rewrite rules public

5b6a862

Port file pruning to session stats rewrites Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

Centralize stat expression binding

e8dd011

Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

gatesn force-pushed the ngates/public-stats-rewrite-rules branch from 5ed375f to e8dd011 Compare June 11, 2026 15:51

gatesn added 3 commits June 11, 2026 14:29

Fix file stats binding for computed expressions

c1d6d94

Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

Fuse checked pruning stats rewrites

468ebfd

Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

Simplify stats binding null handling

cd48674

Signed-off-by: "Nicholas Gates" <nick@nickgates.com> Signed-off-by: Nicholas Gates <nick@nickgates.com>

gatesn marked this pull request as ready for review June 11, 2026 20:10

gatesn requested a review from a team June 11, 2026 20:10

gatesn enabled auto-merge (squash) June 11, 2026 20:11

gatesn added 2 commits June 11, 2026 16:13

Install Java toolchain in CI

52fd443

Signed-off-by: Nicholas Gates <nick@nickgates.com>

Remove legacy stat falsification hooks

4293838

Signed-off-by: Nicholas Gates <nick@nickgates.com>

gatesn changed the title ~~Make stats rewrite rules public~~ Migrate to new stats falsification rules Jun 11, 2026

Remove legacy stat expression hooks

ffbccee

Signed-off-by: Nicholas Gates <nick@nickgates.com>

AdamGS reviewed Jun 12, 2026

View reviewed changes

		@@ -115,14 +119,52 @@ impl FileStatsLayoutReader {
		Ok(result.as_bool().value() == Some(true))

Conversation

gatesn commented Jun 10, 2026

Uh oh!

codspeed-hq Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will improve performance by 13.18%

Performance Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamGS commented Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codspeed-hq Bot commented Jun 11, 2026 •

edited

Loading