cockroachdb / cockroachdb/cockroach

pkg/sql/sem: dependency bloat due to system/debug builtins

Open
#121,203 0 comments 0 reactions 0 assignees View on GitHub
C-cleanup P-3 T-sql-foundations
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.