Skip to content

Commit

Permalink
Introduce basic support for $origin substitution in EntityPathFilter (#…
Browse files Browse the repository at this point in the history
…5517)

### What
- Initial implementation of:
#5288
 - Builds on top of: #5516

This is a very dumb first stab at doing variable substitution.
- Rather than parse the string to extract potential `$vars` it uses the
input environment and blindly tries to substitute all the vars it knows
about (currently only `origin`).
- The biggest downside of this is we get no feedback when a variable
fails to substitute.
- There's just enough future complexity handling edge-cases (e.g.
mismatched `{`, variable-termination, nested substitutions, etc.) that
it might be worth pulling in a proper utility library, though I don't
know if there's an obvious rust ecosystem choice here.

Working through this uncovered some complexities regarding what we store
in different parts of the blueprint. For example, if we do the full
substitution immediately when construction the EntityPathFilter, then we
can't use that Filter to re-create the query with the variable
substitutions.

Additionally, we need to know about these substitutions all the way back
when evaluating whether we want to keep RecommendedSpaceViews because we
need the substitutions applied to do the overlap-checking.

I suspect the direction we might want to go in is to split
EntityPathFilter into a non-substituted representation, from which we
can create a version that executes the substitutions, but I'm not yet
sure what the storage should look like. For example, do we just create
full `EntityPath`s that contain EntityPathParts with "$origin" in them
and then run the substitution on the EntityPath?

### 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 the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5517/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5517/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5517/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5517)
- [Docs
preview](https://rerun.io/preview/82c517d5786790176b78fdfbff4a5e261a25f7f0/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/82c517d5786790176b78fdfbff4a5e261a25f7f0/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
jleibs authored Mar 15, 2024
1 parent 1509aae commit ce407eb
Show file tree
Hide file tree
Showing 19 changed files with 266 additions and 135 deletions.
137 changes: 113 additions & 24 deletions crates/re_log_types/src/path/entity_path_filter.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,21 @@
use std::collections::BTreeMap;

use ahash::HashMap;
use itertools::Itertools as _;

use crate::EntityPath;

/// A set of substitutions for entity paths.
#[derive(Default)]
pub struct EntityPathSubs(pub HashMap<String, String>);

impl EntityPathSubs {
/// Create a new set of substitutions from a single origin.
pub fn new_with_origin(origin: &EntityPath) -> Self {
Self(std::iter::once(("origin".to_owned(), origin.to_string())).collect())
}
}

/// A way to filter a set of `EntityPath`s.
///
/// This implements as simple set of include/exclude rules:
Expand Down Expand Up @@ -54,14 +66,26 @@ impl std::fmt::Debug for EntityPathFilter {
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
pub struct EntityPathRule {
// We need to store the raw expression to be able to round-trip the filter
// when it contains substitutions.
pub raw_expression: String,

pub path: EntityPath,

/// If true, ALSO include children and grandchildren of this path (recursive rule).
pub include_subtree: bool,
}

impl PartialEq for EntityPathRule {
fn eq(&self, other: &Self) -> bool {
self.raw_expression == other.raw_expression
}
}

impl Eq for EntityPathRule {}

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum RuleEffect {
Include,
Expand Down Expand Up @@ -102,8 +126,8 @@ impl EntityPathFilter {
/// The rest of the line is trimmed and treated as an entity path.
///
/// Conflicting rules are resolved by the last rule.
pub fn parse_forgiving(rules: &str) -> Self {
Self::from_query_expressions(rules.split('\n'))
pub fn parse_forgiving(rules: &str, subst_env: &EntityPathSubs) -> Self {
Self::from_query_expressions(rules.split('\n'), subst_env)
}

/// Build a filter from a list of query expressions.
Expand All @@ -114,7 +138,10 @@ impl EntityPathFilter {
/// The rest of the expression is trimmed and treated as an entity path.
///
/// Conflicting rules are resolved by the last rule.
pub fn from_query_expressions<'a>(rules: impl IntoIterator<Item = &'a str>) -> Self {
pub fn from_query_expressions<'a>(
rules: impl IntoIterator<Item = &'a str>,
subst_env: &EntityPathSubs,
) -> Self {
let mut filter = Self::default();

for line in rules
Expand All @@ -128,7 +155,7 @@ impl EntityPathFilter {
_ => (RuleEffect::Include, line),
};

let rule = EntityPathRule::parse_forgiving(path_pattern);
let rule = EntityPathRule::parse_forgiving(path_pattern, subst_env);

filter.add_rule(effect, rule);
}
Expand Down Expand Up @@ -161,15 +188,7 @@ impl EntityPathFilter {
RuleEffect::Include => "+ ",
RuleEffect::Exclude => "- ",
});
if rule.path.is_root() && rule.include_subtree {
// needs special casing, otherwise we end up with `//**`
s.push_str("/**");
} else {
s.push_str(&rule.path.to_string());
if rule.include_subtree {
s.push_str("/**");
}
}
s.push_str(&rule.raw_expression);
s
})
}
Expand Down Expand Up @@ -360,6 +379,7 @@ impl EntityPathRule {
#[inline]
pub fn exact(path: EntityPath) -> Self {
Self {
raw_expression: path.to_string(),
path,
include_subtree: false,
}
Expand All @@ -369,26 +389,40 @@ impl EntityPathRule {
#[inline]
pub fn including_subtree(path: EntityPath) -> Self {
Self {
raw_expression: format!("{path}/**",),
path,
include_subtree: true,
}
}

pub fn parse_forgiving(expression: &str) -> Self {
let expression = expression.trim();
pub fn parse_forgiving(expression: &str, subst_env: &EntityPathSubs) -> Self {
let raw_expression = expression.trim().to_owned();

// TODO(#5528): This is a very naive implementation of variable substitution.
// unclear if we want to do this here, push this down into `EntityPath::parse`,
// or even supported deferred evaluation on the `EntityPath` itself.
let mut expression_sub = raw_expression.clone();
for (key, value) in &subst_env.0 {
expression_sub = expression_sub.replace(format!("${key}").as_str(), value);
expression_sub = expression_sub.replace(format!("${{{key}}}").as_str(), value);
}

if expression == "/**" {
Self {
raw_expression,
path: EntityPath::root(),
include_subtree: true,
}
} else if let Some(path) = expression.strip_suffix("/**") {
} else if let Some(path) = expression_sub.strip_suffix("/**") {
Self {
raw_expression,
path: EntityPath::parse_forgiving(path),
include_subtree: true,
}
} else {
Self {
path: EntityPath::parse_forgiving(expression),
raw_expression,
path: EntityPath::parse_forgiving(&expression_sub),
include_subtree: false,
}
}
Expand Down Expand Up @@ -421,10 +455,12 @@ impl std::cmp::PartialOrd for EntityPathRule {

#[cfg(test)]
mod tests {
use crate::{EntityPath, EntityPathFilter, EntityPathRule, RuleEffect};
use crate::{EntityPath, EntityPathFilter, EntityPathRule, EntityPathSubs, RuleEffect};

#[test]
fn test_rule_order() {
let subst_env = Default::default();

use std::cmp::Ordering;

fn check_total_order(rules: &[EntityPathRule]) {
Expand Down Expand Up @@ -459,19 +495,22 @@ mod tests {
"/world/car/driver",
"/x/y/z",
];
let rules = rules.map(EntityPathRule::parse_forgiving);
let rules = rules.map(|rule| EntityPathRule::parse_forgiving(rule, &subst_env));
check_total_order(&rules);
}

#[test]
fn test_entity_path_filter() {
let subst_env = Default::default();

let filter = EntityPathFilter::parse_forgiving(
r#"
+ /world/**
- /world/
- /world/car/**
+ /world/car/driver
"#,
&subst_env,
);

for (path, expected_effect) in [
Expand All @@ -491,13 +530,59 @@ mod tests {
}

assert_eq!(
EntityPathFilter::parse_forgiving("/**").formatted(),
EntityPathFilter::parse_forgiving("/**", &subst_env).formatted(),
"+ /**"
);
}

#[test]
fn test_entity_path_filter_subs() {
// Make sure we use a string longer than `$origin` here.
// We can't do in-place substitution.
let subst_env = EntityPathSubs::new_with_origin(&EntityPath::from("/annoyingly/long/path"));

let filter = EntityPathFilter::parse_forgiving(
r#"
+ $origin/**
- $origin
- $origin/car/**
+ $origin/car/driver
"#,
&subst_env,
);

for (path, expected_effect) in [
("/unworldly", None),
("/annoyingly/long/path", Some(RuleEffect::Exclude)),
("/annoyingly/long/path/house", Some(RuleEffect::Include)),
("/annoyingly/long/path/car", Some(RuleEffect::Exclude)),
("/annoyingly/long/path/car/hood", Some(RuleEffect::Exclude)),
(
"/annoyingly/long/path/car/driver",
Some(RuleEffect::Include),
),
(
"/annoyingly/long/path/car/driver/head",
Some(RuleEffect::Exclude),
),
] {
assert_eq!(
filter.most_specific_match(&EntityPath::from(path)),
expected_effect,
"path: {path:?}",
);
}

assert_eq!(
EntityPathFilter::parse_forgiving("/**", &subst_env).formatted(),
"+ /**"
);
}

#[test]
fn test_entity_path_filter_subtree() {
let subst_env = Default::default();

let filter = EntityPathFilter::parse_forgiving(
r#"
+ /world/**
Expand All @@ -507,6 +592,7 @@ mod tests {
- /world/city
- /world/houses/**
"#,
&subst_env,
);

for (path, expected) in [
Expand All @@ -533,6 +619,8 @@ mod tests {

#[test]
fn test_is_superset_of() {
let subst_env = Default::default();

struct TestCase {
filter: &'static str,
contains: Vec<&'static str>,
Expand Down Expand Up @@ -594,9 +682,9 @@ mod tests {
];

for case in &cases {
let filter = EntityPathFilter::parse_forgiving(case.filter);
let filter = EntityPathFilter::parse_forgiving(case.filter, &subst_env);
for contains in &case.contains {
let contains_filter = EntityPathFilter::parse_forgiving(contains);
let contains_filter = EntityPathFilter::parse_forgiving(contains, &subst_env);
assert!(
filter.is_superset_of(&contains_filter),
"Expected {:?} to fully contain {:?}, but it didn't",
Expand All @@ -605,7 +693,8 @@ mod tests {
);
}
for not_contains in &case.not_contains {
let not_contains_filter = EntityPathFilter::parse_forgiving(not_contains);
let not_contains_filter =
EntityPathFilter::parse_forgiving(not_contains, &subst_env);
assert!(
!filter.is_superset_of(&not_contains_filter),
"Expected {:?} to NOT fully contain {:?}, but it did",
Expand Down
2 changes: 1 addition & 1 deletion crates/re_log_types/src/path/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ mod parse_path;
pub use component_path::ComponentPath;
pub use data_path::DataPath;
pub use entity_path::{EntityPath, EntityPathHash};
pub use entity_path_filter::{EntityPathFilter, EntityPathRule, RuleEffect};
pub use entity_path_filter::{EntityPathFilter, EntityPathRule, EntityPathSubs, RuleEffect};
pub use entity_path_part::EntityPathPart;
pub use parse_path::PathParseError;

Expand Down
6 changes: 1 addition & 5 deletions crates/re_space_view/src/heuristics.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use re_log_types::EntityPathFilter;
use re_viewer_context::{
ApplicableEntities, IdentifiedViewSystem, RecommendedSpaceView, SpaceViewClass,
SpaceViewSpawnHeuristics, ViewerContext, VisualizerSystem,
Expand Down Expand Up @@ -44,10 +43,7 @@ where
{
None
} else {
Some(RecommendedSpaceView {
root: entity.clone(),
query_filter: EntityPathFilter::single_entity_filter(entity),
})
Some(RecommendedSpaceView::new_single_entity(entity.clone()))
}
})
.collect();
Expand Down
Loading

0 comments on commit ce407eb

Please sign in to comment.