Gordon.hamilton/architecture docs by gord02 · Pull Request #170 · DataDog/substrait-explain

gord02 · 2026-07-02T19:25:20Z

Description

This docs adds additional docs to help better understand the code structure and flow of the substrait-explain repo.

wackywendell · 2026-07-06T14:42:00Z

Can we clarify: who is the audience for this document? Human, AI, or both? Under what circumstances would they read it? What purpose does it serve that is not covered by the docs?

I ask in part because this is very heavy on implementation details, which are likely to change - and then this document would be out of date, or need to be carefully maintained. In general, this kind of documentation can be helpful, but I think it might be best kept as part of the code's direct documentation - that helps to ensure its accurate, and then it can both be read independently (via cargo doc) or directly from the code. For AI purposes, it might make sense to link to that documentation, and/or direct the AI to build the docs (possibly with cargo doc --no-deps --all-features --document-private-items to include all items) and see those.

wackywendell

Overall, I think there's a lot of good content here, that would be good to add to our docs.

However, I would suggest that instead of adding a new document, we instead add it to the existing docs - as doc comments on modules / structs / functions, wherever they are specific to that, and to CONTRIBUTING.md and/or AGENTS.md directing them to those docs.

Putting the docs inline that is useful for a couple reasons:

You get cargo doc formatting and links, and you can make examples that compile and run in tests
You get cargo doc code generation
It's easier for it to stay up-to-date with the code; a comment on a module can't refer to the "wrong" module or path if its part of that module/path, and cargo doc will warn about invalid links. Also, contributors are more likely to notice doc comments in the file they are already editing.

That said - there's good content here, and plenty of places in our docs (e.g. here) that are low on documentation and could use updates; I'd love to see much of this reframed into a PR that adds / extends those comments!

wackywendell · 2026-07-06T14:28:44Z

+
+### `textify/rels.rs` — Relation Output
+
+At the top level, a Substrait `Plan` contains `PlanRel` entries. Each `PlanRel` is either a `Root` or a regular `Rel`. The textifier handles `Root` separately in the `Textify for RelRoot` implementation. Regular protobuf relation types such as `Read`, `Filter`, and `Project` are converted into the `Relation` shape before rendering. Proto relations can be represented by their arguments, columns output mapping(Emit), addenda(advanced extensions), and children; which we store in the Relation internal type.


This is describing the code, but not describing the architecture: what does a PlanRel represent? What does it mean in terms of higher abstractions?

children; which we store

Can we fix the grammar here as well? Anything following a semicolon should be a self-contained clause (i.e. would be a complete sentence on its own)

wackywendell · 2026-07-15T17:02:54Z

+
+---
+
+## Module Map


This is roughly what is shown in the docs (here)[https://datadoghq.dev/substrait-explain/substrait_explain/index.html#modules]. Some of those descriptions could be improved; perhaps we can do that on them directly?

wackywendell · 2026-07-15T17:03:17Z

+
+---
+
+## `src/json.rs` — JSON to Proto


This would be great to clarify in the doc comment for src/json.rs

wackywendell · 2026-07-15T17:04:07Z

+7. textify/extensions.rs — called from rels for custom extension argument values (Value::ExtensionArgument, Value::ExtColumn)
+8. textify/addenda.rs — called from rels for + Enh:, + Opt:, + Ext: lines, which call into extensions.rs for rendering the decoded args
+
+### Entry Point


If this isn't clear in API.md (what is shown in the main index.html for the Rust docs), it should be clarified!

wackywendell · 2026-07-15T17:04:27Z

+
+The public entry points in `lib.rs` are `format()`, `format_with_options()`, and `format_with_registry()`. All three ultimately call `format_with_registry`, which is the real implementation. `format()` and `format_with_options()` are convenience wrappers that supply a default empty `ExtensionRegistry`. The registry is always required because advanced extensions: Enhancements(`+ Enh:`), Optimizations(`+ Opt:`), and extension relations(LeafRel, SingleRel, MultiRel), carry their payload as a `google.protobuf.Any` blob. The registry knows how to decode those blobs into readable text.
+
+### `textify/plan.rs` — Top-Level Output


As above - let's clarify this in the docs themselves

wackywendell · 2026-07-15T17:07:04Z

+
+### Entry Point
+
+The public entry points in `lib.rs` are `parse()` and `parse_with_registry()`. Unlike the textify side where `format()` and `format_with_options()` are thin wrappers that delegate to `format_with_registry`, here `parse()` does not delegate to `parse_with_registry()` — they are separate implementations.


As above - should go in API.md and/or lib.rs docstring

wackywendell · 2026-07-15T17:11:48Z

+
+The parser carries an `ExtensionRegistry` for the same reason as the textifier: advanced extension payloads (`+ Enh:`, `+ Opt:`, `+ Ext:`, and extension relations) are `google.protobuf.Any` blobs. `parse()` uses a default empty registry through `Parser::new()`, while `parse_with_registry()` supplies a caller-provided registry. The difference is in error handling: when textifying, an unregistered advanced extension can be represented as a soft error token output. When parsing, an unregistered advanced extension is a hard `ParseError::UnregisteredExtension`, because the parser cannot create a valid `Any` payload without the registered type. Plans with advanced extensions therefore need `parse_with_registry()`.
+
+### `expression_grammar.pest` — PEG Grammar


This file doesn't have auto-generated docstrings, but is imported / turned into this struct. The documentation here would fit well on that struct and/or on src/parser/structural.rs or src/parser/common.rs (e.g. the section markers)

wackywendell · 2026-07-15T17:45:03Z

+
+---
+
+## Testing


This is also interesting; in-file doc comments would make sense for most of this, for the same reason as the rest, except cargo doc doesn't generate docs for #[cfg(test)] generally.

I made an issue for the general setup: #184

In the meantime - I think you could add this as comments on the modules, and perhaps add a ## Testing section to CONTRIBUTING.md that references those files.

wackywendell · 2026-07-15T17:57:06Z

+
+## Examples and Reference Plans
+
+`examples/` contains runnable Cargo examples (`cargo run --example <name>`) demonstrating the public API:


These should also be covered in the individual files, although perhaps also in examples/README.md with links from there.

wackywendell · 2026-07-15T18:15:56Z

+- `advanced_usage.rs` covers output options
+- `extensions.rs` shows the full custom extension handler pattern end-to-end — defining a type, implementing `Explainable`, registering it, and round-tripping through the registry.
+
+`example-plans/` contains sample `.substrait` text files at different feature levels — basic relations, scalar functions, user-defined types. These are useful as manual test inputs and as reference when working on the parser or textifier.


🤔 Do we run these as tests? 🤔 We should... and we should document whether we do either way; probably in example-plans/README.md...

gord02 added 6 commits June 30, 2026 19:52

feat: adding architecture docs

c27ded5

feat: adding the parser files description to file

bd1fa68

docs: adding parsing description

b8fda36

docs: updating extensions section

1610706

docs: adding testing explanation

2dbbcee

docs: more edits

103afe3

gord02 requested review from a team and wackywendell as code owners July 2, 2026 19:25

wackywendell requested changes Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Gordon.hamilton/architecture docs#170

Gordon.hamilton/architecture docs#170
gord02 wants to merge 6 commits into
mainfrom
gordon.hamilton/architecture-docs

gord02 commented Jul 2, 2026

Uh oh!

wackywendell commented Jul 6, 2026

Uh oh!

wackywendell left a comment

Uh oh!

wackywendell Jul 6, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

wackywendell Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		### `textify/rels.rs` — Relation Output

		At the top level, a Substrait `Plan` contains `PlanRel` entries. Each `PlanRel` is either a `Root` or a regular `Rel`. The textifier handles `Root` separately in the `Textify for RelRoot` implementation. Regular protobuf relation types such as `Read`, `Filter`, and `Project` are converted into the `Relation` shape before rendering. Proto relations can be represented by their arguments, columns output mapping(Emit), addenda(advanced extensions), and children; which we store in the Relation internal type.


		The public entry points in `lib.rs` are `format()`, `format_with_options()`, and `format_with_registry()`. All three ultimately call `format_with_registry`, which is the real implementation. `format()` and `format_with_options()` are convenience wrappers that supply a default empty `ExtensionRegistry`. The registry is always required because advanced extensions: Enhancements(`+ Enh:`), Optimizations(`+ Opt:`), and extension relations(LeafRel, SingleRel, MultiRel), carry their payload as a `google.protobuf.Any` blob. The registry knows how to decode those blobs into readable text.

		### `textify/plan.rs` — Top-Level Output


		### Entry Point

		The public entry points in `lib.rs` are `parse()` and `parse_with_registry()`. Unlike the textify side where `format()` and `format_with_options()` are thin wrappers that delegate to `format_with_registry`, here `parse()` does not delegate to `parse_with_registry()` — they are separate implementations.


		The parser carries an `ExtensionRegistry` for the same reason as the textifier: advanced extension payloads (`+ Enh:`, `+ Opt:`, `+ Ext:`, and extension relations) are `google.protobuf.Any` blobs. `parse()` uses a default empty registry through `Parser::new()`, while `parse_with_registry()` supplies a caller-provided registry. The difference is in error handling: when textifying, an unregistered advanced extension can be represented as a soft error token output. When parsing, an unregistered advanced extension is a hard `ParseError::UnregisteredExtension`, because the parser cannot create a valid `Any` payload without the registered type. Plans with advanced extensions therefore need `parse_with_registry()`.

		### `expression_grammar.pest` — PEG Grammar


		## Examples and Reference Plans

		`examples/` contains runnable Cargo examples (`cargo run --example <name>`) demonstrating the public API:


		---

		## Testing

Uh oh!

Conversation

gord02 commented Jul 2, 2026

Description

Uh oh!

wackywendell commented Jul 6, 2026

Uh oh!

wackywendell left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants