cockroachdb / cockroachdb/cockroach

scbuild: astAnnotator doesn't duplicate the AST in the right way

Open
#101,977 1 comment 0 reactions 0 assignees View on GitHub
A-schema-changes C-bug T-sql-foundations
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

This code:

```go
func newAstAnnotator(original tree.Statement) (*astAnnotator, error) {
// Clone the original tree by re-parsing the input back into an AST. We need
// to keep tagged dollar quotes in case they're necessary to parse the
// original statement.
statement, err := parser.ParseOne(tree.AsStringWithFlags(original, tree.FmtTagDollarQuotes))
```

Has multiple problems:

1. we do not always have a guarantee that pretty-print the entire statement will return something that can be parsed.
- generally, we nurture this guarantee only for scalar expression (for dsql execution), but not as much for entire statements
- yes, we have some unit testing to get there, but the unit testing is often incomplete (e.g. #101978)
- a simple deep copy of the tree would achieve the necessary outcome without placing a correctness requirement on pretty-printing
2. it's expensive! The computation of pretty-printing is extremely hungry wrt memory allocations and puts a lot of pressure on the memory GC. A simple deep copy of the tree would be sufficient here.
3. arguably, it doesn't need to be performed upfront; we only need the copy the first time the AST is modified (i.e. lazy copy)

Jira issue: CRDB-27196

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.