Skip to content

Commit

Permalink
Fix bug when joining cleared optional components (#3726)
Browse files Browse the repository at this point in the history
### What
Resolves the second issue identified from:
 - #3711
 
Even with the fix from:
 - #3720
 
We could still reproduce an error with code such as:
```
"""Log a simple line strip."""
import rerun as rr

rr.init("rerun_example_line_strip3d", spawn=True)

points = [
    [0, 0, 0],
    [0, 0, 1],
    [1, 0, 0],
    [1, 0, 1],
    [1, 1, 0],
    [1, 1, 1],
    [0, 1, 0],
    [0, 1, 1],
]

rr.log("strip", rr.LineStrips3D([points], radii=[]))
```

Which would still display no lines.

It turns out the single-row joining iterator introduced a bug when a
component is cleared. This yields an empty cell for the component, but
still passes the key-based check that was being done, and then
subsequently yields no items.

This PR adds a unit-test reproing the issue, a debug_assertion where we
were previously violating an assumption, and now guards against the
condition.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3726) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3726)
- [Docs
preview](https://rerun.io/preview/7434604b10e7fb047e289dfd5ff1352ba23eadef/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/7434604b10e7fb047e289dfd5ff1352ba23eadef/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
  • Loading branch information
jleibs authored Oct 9, 2023
1 parent b50fac9 commit 9d5feae
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 1 deletion.
21 changes: 20 additions & 1 deletion crates/re_query/src/archetype_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,13 @@ impl<A: Archetype> ArchetypeView<A> {

let component = self.components.get(&C::name());

if let Some(component) = component {
// If the component is found and not empty, run the joining iterator on it.
// Otherwise just output nulls of the length of the primary.
// Note that this guard is specifically a precondition of the inner optimization
// for matched instance keys which will debug_assert if a zero-length component is
// referenced there.
let is_empty = component.map_or(true, |c| c.is_empty());
if let (Some(component), false) = (component, is_empty) {
// NOTE(1): Autogenerated instance keys are interned behind datacells.
// If two or more rows in the store share the same keys, then they will share
// also the same cells.
Expand All @@ -356,7 +362,20 @@ impl<A: Archetype> ArchetypeView<A> {
// faster iterator.
// - If the data isn't the same, the cost of the comparison will be dwarfed by the cost
// of the join anyway.

if self.required_comp().instance_keys == component.instance_keys {
// This fast iterator is assumed to match the length of the
// primary component We shouldn't hit this since the store
// should enforce matched lengths for non-empty components, and
// the outer if-guard should keep us from reaching this in the
// case of an empty component.
// TODO(#1893): This assert and the implementation both need to
// be addressed when we allow single rows containing splats.
debug_assert!(
self.required_comp().instance_keys.num_instances()
== component.values.num_instances()
);

// NOTE: A component instance cannot be optional in itself, and if we're on this
// path then we know for a fact that both batches can be intersected 1-to-1.
// Therefore there cannot be any null values, therefore we can go through the fast
Expand Down
83 changes: 83 additions & 0 deletions crates/re_query/tests/archetype_visit_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,3 +285,86 @@ fn joint_visit() {
assert_eq!(positions, positions_out);
assert_eq!(expected_colors, colors_out);
}

#[test]
fn joint_visit_with_empty() {
let positions = vec![
Position2D::new(1.0, 2.0), //
Position2D::new(3.0, 4.0),
Position2D::new(5.0, 6.0),
Position2D::new(7.0, 8.0),
Position2D::new(9.0, 10.0),
];

let shared_ids = InstanceKey::from_iter(0..5);

let colors: Vec<Color> = vec![];

let positions_comp =
ComponentWithInstances::from_native(shared_ids.clone(), positions.clone()).unwrap();
let colors_comp = ComponentWithInstances::from_native(shared_ids, colors).unwrap();

let arch_view =
ArchetypeView::<Points2D>::from_components(RowId::ZERO, [positions_comp, colors_comp]);

let positions_out = arch_view
.iter_required_component::<Position2D>()
.unwrap()
.collect_vec();
let colors_out = arch_view
.iter_optional_component::<Color>()
.unwrap()
.collect_vec();
assert_eq!(positions_out.len(), colors_out.len());

let expected_colors = vec![None, None, None, None, None];

assert_eq!(positions, positions_out);
assert_eq!(expected_colors, colors_out);
}

#[test]
fn joint_visit_with_splat() {
let positions = vec![
Position2D::new(1.0, 2.0), //
Position2D::new(3.0, 4.0),
Position2D::new(5.0, 6.0),
Position2D::new(7.0, 8.0),
Position2D::new(9.0, 10.0),
];

let shared_ids = InstanceKey::from_iter(0..5);

let color = Color::from(0xff000000);
let colors: Vec<Color> = vec![color];

let positions_comp =
ComponentWithInstances::from_native(shared_ids, positions.clone()).unwrap();
// TODO(#1893): Replace the instance_keys with with shared_ids.
let colors_comp =
ComponentWithInstances::from_native(vec![InstanceKey::SPLAT], colors).unwrap();

let arch_view =
ArchetypeView::<Points2D>::from_components(RowId::ZERO, [positions_comp, colors_comp]);

let positions_out = arch_view
.iter_required_component::<Position2D>()
.unwrap()
.collect_vec();
let colors_out = arch_view
.iter_optional_component::<Color>()
.unwrap()
.collect_vec();
assert_eq!(positions_out.len(), colors_out.len());

let expected_colors = vec![
Some(color),
Some(color),
Some(color),
Some(color),
Some(color),
];

assert_eq!(positions, positions_out);
assert_eq!(expected_colors, colors_out);
}

0 comments on commit 9d5feae

Please sign in to comment.