Revise Lenses API and definitions to include scatter columns #11871

grtlr · 2025-11-12T10:17:33Z

What

This PR revises the Lenses API and specification by introducing different types of lenses that are centered around the time columns:

OneToOne lenses have the same amount of output rows as their input column.
Static lenses don't have any time columns attached to them.
OneToMany lenses produce multiple rows for any given row in their input column.

These kind of lenses are accessed in the LensBuilder via columns, static_columns, and scatter_columns methods, which create scopes that can be used to bundle columns together.

Although the concept of chunks is not exposed in the Lenses API, under the hood all columns (time or component) specified in a scope (LensKind) will end up in the same chunk.

Context

The new scatter columns operating mode is required for "plural" MCAP messages that have multiple timestamps defined in a single message. An example for this is foxglove.FrameTransforms.

Copilot

Pull Request Overview

This PR refactors the Lenses API to support three distinct lens types based on row transformation patterns: OneToOne (1:1 mapping), Static (timeless data), and ToMany (1:N scatter). The new builder-based API uses output_columns, output_static_columns, and output_scatter_columns methods to create scoped column groups, replacing the previous single-method approach.

Key Changes:

Introduced LensKind enum to distinguish between lens transformation types
Added builder pattern with scoped output methods for different lens modes
Implemented Explode transformation for flattening nested lists in scatter operations

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
examples/rust/lenses/src/main.rs	Updated lens construction to use new builder API with scoped column methods
crates/utils/re_arrow_combinators/tests/util.rs	Added helper utility for displaying arrays as record batches in tests
crates/utils/re_arrow_combinators/tests/transform.rs	Refactored to use shared utility module
crates/utils/re_arrow_combinators/tests/snapshots/*.snap	Added snapshot test files for explode functionality
crates/utils/re_arrow_combinators/tests/explode.rs	New test file for explode transformation
crates/utils/re_arrow_combinators/src/reshape.rs	Implemented `Explode` transformation for scattering list elements
crates/top/re_sdk/src/lenses/snapshots/*.snap	Added snapshots for scatter column tests
crates/top/re_sdk/src/lenses/op.rs	Minor import restructuring
crates/top/re_sdk/src/lenses/mod.rs	Updated exports to include new builder types
crates/top/re_sdk/src/lenses/builder.rs	New builder API with `ColumnsBuilder`, `StaticColumnsBuilder`, and `ScatterColumnsBuilder`
crates/top/re_sdk/src/lenses/ast.rs	Major refactoring to support new lens types and transformation logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/top/re_sdk/src/lenses/builder.rs

Rethink target entity paths Clean up terminology: `Timeline` vs `Time`

github-actions · 2025-11-12T10:41:53Z

Web viewer built successfully.

Result	Commit	Link	Manifest
✅	`a8e1695`	https://rerun.io/viewer/pr/11871	`+nightly` `+main`

View image diff on kitdiff.

^{Note: This comment is updated whenever you push a commit.}

emilk

I only have a superficial understanding of lenses, so I mostly reviewed the code.

It mostly looks good except the pervasive logging of errors instead of returning them, which is very unidiomatic. If we're going that route I want a good motivation for it, and tests that include checking the log output for any errors!

emilk · 2025-11-12T16:08:49Z

crates/top/re_sdk/src/lenses/ast.rs

+    ToMany {
+        target_entity: TargetEntity,
+        components: Vec<ComponentOutput>,
+        timelines: Vec<TimeOutput>,


Does this mean timelines is never empty? Should we maybe use vec1 for it?

Sorry the doc comment was confusing here. Time columns defined via Lenses are optional.

Regarding vec1: ~~Might make sense for component columns, but these structs would be backing a potential future config language so that would make them weird to use?~~ A just saw that vec1 has a serde feature.

crates/top/re_sdk/src/lenses/ast.rs

emilk · 2025-11-12T16:15:55Z

crates/top/re_sdk/src/lenses/ast.rs

+                re_log::error!(
+                    "Lens operations failed for component columns '{}': {err}",
+                    output.component_descr
+                );


Are we sure we should not abort the operation if this happens?

emilk · 2025-11-12T16:16:49Z

crates/top/re_sdk/src/lenses/ast.rs

+    } else {
+        re_log::error_once!(
+            "Output for timeline '{timeline_name}' must produce data type {}",
+            DataType::List(Arc::new(Field::new_list_field(DataType::Int64, false))),


yeeshus arrow-rs can be verbose sometimes…

emilk · 2025-11-12T16:31:58Z

crates/utils/re_arrow_combinators/src/reshape.rs

+                    let inner_offsets = inner_list.value_offsets();
+
+                    for j in start..end {
+                        new_validity.push(!inner_list.is_null(j));


Suggested change

new_validity.push(!inner_list.is_null(j));

new_validity.push(inner_list.is_valid(j));

emilk · 2025-11-12T16:32:37Z

crates/utils/re_arrow_combinators/src/reshape.rs

+    type Source = ListArray;
+    type Target = ListArray;
+
+    fn transform(&self, source: &Self::Source) -> Result<Self::Target, Error> {


Suggested change

fn transform(&self, source: &Self::Source) -> Result<Self::Target, Error> {

fn transform(&self, source: &Self::Source) -> Result<Self::Target, Error> {

re_tracing::profile_function!();

emilk · 2025-11-12T16:34:01Z

crates/utils/re_arrow_combinators/tests/snapshots/explode__primitives.snap

@@ -0,0 +1,21 @@
+---
+source: crates/utils/re_arrow_combinators/tests/explode.rs
+expression: "format!(\"{}\", DisplayRB(result))"


It would be a lot easier to check these tests if these snapshot tests included the input.

Alternatively, put the snapshot output into the test files with inline snapshots

emilk · 2025-11-12T16:34:24Z

crates/utils/re_arrow_combinators/tests/explode.rs

+    let explode = Explode;
+    let result = explode.transform(&input).unwrap();
+
+    insta::assert_snapshot!("primitives", format!("{}", DisplayRB(result)));


Consider using https://docs.rs/insta/latest/insta/#inline-snapshots

emilk · 2025-11-12T16:35:58Z

crates/top/re_sdk/src/lenses/ast.rs

+            Err(err) => {
+                re_log::error_once!(
+                    "Failed to replicate timeline '{}' for entity '{entity_path}': {err}",
+                    timeline_name
+                );
+                return None;


In some places you log errors and continue, but here you log an error and abort. Why not just return an Err instead?

grtlr added the exclude from changelog PRs with this won't show up in CHANGELOG.md label Nov 12, 2025

grtlr requested a review from Copilot November 12, 2025 10:19

Copilot AI reviewed Nov 12, 2025

View reviewed changes

crates/top/re_sdk/src/lenses/builder.rs Outdated Show resolved Hide resolved

grtlr force-pushed the grtlr/lenses-v2 branch 2 times, most recently from 23173e5 to 4abf140 Compare November 12, 2025 10:28

grtlr added 2 commits November 12, 2025 11:40

Implement Explode transformation in re_arrow_combinators

987aee4

Implement new lens kinds OneToOne, Static, and OneToMany

01ad210

Rethink target entity paths Clean up terminology: `Timeline` vs `Time`

grtlr force-pushed the grtlr/lenses-v2 branch from 4abf140 to 01ad210 Compare November 12, 2025 10:41

grtlr added 2 commits November 12, 2025 11:48

Adapt mcap_protobuf example

303389a

fmt

5ee9171

oxkitsune self-requested a review November 12, 2025 13:09

emilk requested changes Nov 12, 2025

View reviewed changes

grtlr added 3 commits November 13, 2025 08:25

Extract variants into structs

6662a7b

Better error handling in builder.rs

477fbfc

Contextualized errors

a8e1695

	new_validity.push(!inner_list.is_null(j));
	new_validity.push(inner_list.is_valid(j));

	fn transform(&self, source: &Self::Source) -> Result<Self::Target, Error> {
	fn transform(&self, source: &Self::Source) -> Result<Self::Target, Error> {
	re_tracing::profile_function!();

Revise Lenses API and definitions to include scatter columns #11871

Are you sure you want to change the base?

Revise Lenses API and definitions to include scatter columns #11871

Conversation

grtlr commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Context

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

github-actions bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grtlr Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grtlr commented Nov 12, 2025 •

edited

Loading

github-actions bot commented Nov 12, 2025 •

edited

Loading

emilk left a comment •

edited

Loading

grtlr Nov 13, 2025 •

edited

Loading