cockroachdb / cockroachdb/cockroach
pkg/sql/sem: dependency bloat due to system/debug builtins
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
It feels like `EvalContext` is getting a little out of hand, with more and more stuff -- particularly interfaces -- being added just because one builtin for one feature or _internal debugging tool_ happens to need to invoke some code from across the codebase. Ditto, for crdb_internal.go and vtables, though pkg/sql was already a dumping ground megapackage so it is less noticeable there.
Really, all that `sem` should need to know about is that the builtin with X name has Y _shape_ -- these inputs, then some code will run, and these outputs. It shouldn't need to know all of the types/interfaces/etc that the code that will run to produce those outputs depends on. Eg because a builtin needs `GetStreamIngestManager` that gets you to `eval.StreamIngestManager` and now we're depending on `streampb`, so any proto change all the way over in PCR, to add a new field that is _only used to pass data between pieces of the implementation of PCR_ now invalidates pkg/sql/sem, and there are many more examples like this.
We should move the code that implements all the builtins that are depending on non-sem/non-tree types out of sem, by adding a single interface to `EvalCtx` that has a go function def in it for every injected builtin, where the signatures in this interface would only be in terms of datrums -> datums, without any non-`sem`/`tree` types visible. Then in pkg/sql we could do:`EvalCtx{Builtins: Builtins{planner: planner}}` and `struct Builtins` would implement all the builtins in terms with access to planner/execCfg/etc, instead needing added windows into those types from pkg/sql/sem?
We could then go even further and have some `var InjectedBuiltins struct { var FooBuiltin func(p Planner) []tree.Datum; var BarBuiltin func(...)}` that other _packages
Jira issue: CRDB-37135
Epic CRDB-60948
Contributor guide
Assessment
This issue has not been assessed yet.