Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce basic support for $origin substitution in EntityPathFilter #5517

Merged
merged 7 commits into from
Mar 15, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Mar 15, 2024

What

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 EntityPaths that contain EntityPathParts with "$origin" in them and then run the substitution on the EntityPath?

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@jleibs jleibs added 🟦 blueprint The data that defines our UI include in changelog labels Mar 15, 2024
@jleibs jleibs changed the title Jleibs/support for origin Introduce basic support for $origin substitution in EntityPathFilter Mar 15, 2024
@jleibs jleibs marked this pull request as ready for review March 15, 2024 00:39
@jleibs jleibs mentioned this pull request Mar 15, 2024
5 tasks
emilk pushed a commit that referenced this pull request Mar 15, 2024
### What
 - Builds on top of #5517

Now that we have origin-based content defaults, I'm happy with this
blueprint.

```python
    blueprint = rrb.Blueprint(
        rrb.Horizontal(
            rrb.Grid(
                rrb.BarChartView(name="Bar Chart", origin="/bar_chart"),
                rrb.TimeSeriesView(name="Curves", origin="/curves"),
                rrb.TimeSeriesView(name="Trig", origin="/trig"),
                rrb.TimeSeriesView(name="Classification", origin="/classification"),
            ),
            rrb.TextDocumentView(name="Description", origin="/description"),
            column_shares=[2, 1],
        ),
        rrb.SelectionPanel(expanded=False),
        rrb.TimePanel(expanded=False),
    )
```


![image](https://github.com/rerun-io/rerun/assets/3312232/fad8a49f-5541-420f-b382-f27509bbed81)


### 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/5518/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5518/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/5518/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/5518)
- [Docs
preview](https://rerun.io/preview/eab89b24fcee4cf40f47c7f0da6352c9dc4bf45d/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/eab89b24fcee4cf40f47c7f0da6352c9dc4bf45d/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

Comment on lines 390 to 408
// TODO(jleibs): 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 {
expression_sub = expression_sub.replace(format!("${key}").as_str(), value);
expression_sub = expression_sub.replace(format!("${{{key}}}").as_str(), value);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One failure to call out it that $foo will also match $foobar.

But yeah, solving this in general is a bit tricky, since parsing ${foo}/bar as an entity path currently results in a warning being printed about unescaped special characters. If we remove that warning though, we could parse, then substitute.

Or we can push the subst_env into EntityPath::parse_forgiving and handle it there.

I think this is good enough as a first step though, but let's add a TODO linking to an actual issue listing the shortcomings and potential solutions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, a better solution occurred to me: if we limit substitutions to the start of each expression, we can make something simple that works well: matching either the entire expression ($origin) or stripping the start ($origin/).

Motivation: all EntityPath:s are absolute, so it only makes sense to substitute the start of an expression!

If we do that, I think we should change subst_env: &HashMap<String, String> to prefix_subst_env: &HashMap<String, EntityPath> for clarity

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the rationale, though it feels a bit unnecessarily restrictive to me for marginal value.

For origin it obviously sense, but I think variable substitution is enough of a generic escape hook to just let users do the thing they are used to. It's very low cost for us to in the near future add an optional env parameter to SpaceViews that users can pass in.

This then become a natural stepping stone to re-usable templates in blueprints where you just re-use the exact same space-view, but set up a different substitution env.

crates/re_log_types/src/path/entity_path_filter.rs Outdated Show resolved Hide resolved
Base automatically changed from jleibs/query_expression_array to main March 15, 2024 11:59
jleibs added 5 commits March 15, 2024 08:00
### What
 - Builds on top of #5517

Now that we have origin-based content defaults, I'm happy with this
blueprint.

```python
    blueprint = rrb.Blueprint(
        rrb.Horizontal(
            rrb.Grid(
                rrb.BarChartView(name="Bar Chart", origin="/bar_chart"),
                rrb.TimeSeriesView(name="Curves", origin="/curves"),
                rrb.TimeSeriesView(name="Trig", origin="/trig"),
                rrb.TimeSeriesView(name="Classification", origin="/classification"),
            ),
            rrb.TextDocumentView(name="Description", origin="/description"),
            column_shares=[2, 1],
        ),
        rrb.SelectionPanel(expanded=False),
        rrb.TimePanel(expanded=False),
    )
```


![image](https://github.com/rerun-io/rerun/assets/3312232/fad8a49f-5541-420f-b382-f27509bbed81)


### 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/5518/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5518/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/5518/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/5518)
- [Docs
preview](https://rerun.io/preview/eab89b24fcee4cf40f47c7f0da6352c9dc4bf45d/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/eab89b24fcee4cf40f47c7f0da6352c9dc4bf45d/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@jleibs jleibs force-pushed the jleibs/support_for_origin branch from 735c12c to f7b3380 Compare March 15, 2024 14:17
@jleibs jleibs merged commit ce407eb into main Mar 15, 2024
34 checks passed
@jleibs jleibs deleted the jleibs/support_for_origin branch March 15, 2024 14:49
@emilk emilk changed the title Introduce basic support for $origin substitution in EntityPathFilter Introduce basic support for $origin substitution in EntityPathFilter Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants