opt: introduce optimizer expression trees and build them from TypedExprs

[RaduBerinde]

This change brings in a subset of
https://github.com/petermattis/opttoy/tree/master/v3

This change introduces:

• the expr tree: cascades-style optimizers operate on expression
  trees which can represent both scalar and relational expressions;
  this is a departure from the way we represent expressions and
  statement (sem/tree) so we need a new tree structure.

• scalar operators: initially, we focus only on scalar expressions.

• building an expr tree from a sem/tree.TypedExpr.

• opt version of logic tests

See the RFC in #19135 for more context on the optimizer.

This is the first step of an initial project related to the optimizer:
generating index constraints from scalar expressions. This will be a
rewrite of the current index constraint generation code (which has
many problems, see #6346). Roughly, the existing
makeIndexConstraints will call into the optimizer with a TypedExpr
and the optimizer will return index constraints.

Release note: None

[cockroach-teamcity]

This change is 

[petermattis]

---

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/expr.go, line 18 at r1 (raw file):

// expr is a unified interface for both relational and scalar expressions in a
// query. TODO(radu): currently, it only supports scalar expressions.

Either pull in some of the additional commentary from opttoty or add another TODO about expanding the comments. The opttoy comments probably need to be rewritten. I'm fine with a TODO.

---

pkg/sql/opt/expr.go, line 19 at r1 (raw file):

// expr is a unified interface for both relational and scalar expressions in a
// query. TODO(radu): currently, it only supports scalar expressions.
type expr struct {

I imagine we'll have to export expr at some point, though we can cross that bridge when we reach it.

---

pkg/sql/opt/opt_test.go, line 55 at r1 (raw file):

var (
	logicTestData    = flag.String("d", "testdata/[^.]*", "test data glob")
	rewriteTestFiles = flag.Bool("rewrite", false, "")

Time to copy over the help text from rewrite-results-in-testfiles. I was lazy in not doing that before.

---

pkg/sql/opt/tree_print.go, line 30 at r1 (raw file):

//    |- child2
//
type treePrinter struct {

The name treePrinter is slightly confusing given the sql/sem/tree package. Perhaps this should be exprPrinter. Or perhaps exprDebugPrinter, though that is a bit verbose.

---

pkg/sql/opt/tree_print.go, line 34 at r1 (raw file):

	// We maintain the rows accumulated so far.
	// When a new child is adedd (e.g. child2 above), we may have to go back up

s/adedd/added/g

---

pkg/sql/opt/testdata/build-scalar, line 11 at r1 (raw file):

const (1) (type: int)

Are the 2 blank lines intentional? If yes, you're not consistent below.

---

Comments from Reviewable

[knz]

I'm sorry but the commit message doesn't cut it.
6 months down the line, or the next batch of hires, will not understand anything about the backstory.
The commit message must explain what is going on without appealing to implicit context

So:

• expand the commit title
• summarize what the different types are for in the commit message
• remind the reader where this is going

[RaduBerinde]

summarize what the different types are for in the commit message

What types? The types in the code should be documented in the code, not in commit messages.

remind the reader where this is going

The commit message already includes this paragraph:

The plan is to use this framework to rewrite index constraint generation
(#6346). The sql code will pass a `TypedExpr` to the optimizer and have it
compute the index constraints.

[petermattis]

The commit message must explain what is going on without appealing to implicit context

That's a high bar to clear and not one that I would personally impose.

[knz]

Okay let me try that for you:

sql: introduce new data structures to represent logical plans

This patch aims introduces new data structures that will facilitate 
the later introduction of a new logical plan optimization infrastructure.

This is achieved by introducing:
- a new generic `expr` type intended to represent any node in a logical plan
- a concrete enumeration of constant values that denote `expr` sub-types, 
  that differentiate between logical plan operators, sufficient to encode
  *scalar* expressions
- to exercise the code, a recursive function `buildScalar` which takes a 
  `tree.TypedExpr` tree and converts it to an `expr` tree
- a couple of a tests that check these new features.

See the RFC [...ref...] for context.

Release note: None

[knz]

Also it is a mistake to place the expr struct definitions in the opt package.

It should go to sql/sem/expr.

[knz]

Note that I insist that a competent reviewer must be able to check and understand this code without looking at opttoy and even know that it exists. There are a few more shortcomings, I have more comments coming soon.

[petermattis]

Note that I insist that a competent reviewer must be able to check and understand this code without looking at opttoy and even know that it exists. There are a few more shortcomings, I have more comments coming soon.

Where are we going to find such an engineer? Or are you going to wipe knowledge of opttoy from your memory?

There are definite shortcomings to the code and some design decisions that won't be clear without additional context. IMO, the hurdles you appear to be putting up will not be justified by the benefit (if any) they provide. On the downsides of the hurdles are significant. Clearly you feel very strongly about this and I imagine discussing in PR comments will be fraught. Let's find time to discuss in person.

[knz]

Reviewed 8 of 9 files at r1.
Review status: 8 of 9 files reviewed at latest revision, 21 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/build.go, line 1 at r1 (raw file):

// Copyright 2017 The Cockroach Authors.

This file belongs to a new package sql/sem/expr.

---

pkg/sql/opt/build.go, line 80 at r1 (raw file):

}

const exprAllocChunk = 16

Document these two constants.

---

pkg/sql/opt/expr.go, line 1 at r1 (raw file):

// Copyright 2017 The Cockroach Authors.

This file belongs to a new package sql/sem/expr.

---

pkg/sql/opt/expr.go, line 24 at r1 (raw file):

	// dependent.
	children []*expr
	// Scalar properties.

Extend the comment to remind the reader what are scalar properties.

---

pkg/sql/opt/misc.go, line 19 at r1 (raw file):

import "fmt"

func panicf(format string, args ...interface{}) {

This function should be added in a separate commit.

---

pkg/sql/opt/operator.go, line 1 at r1 (raw file):

// Copyright 2017 The Cockroach Authors.

This file belongs to a new package sql/sem/expr.

---

pkg/sql/opt/operator.go, line 26 at r1 (raw file):

	// TODO(radu): no relational operators yet.

	// Scalar operators

Every operator below exists is rather self-explanatory. The first 4 are non-trivial however and should be further explained in comments.

---

pkg/sql/opt/operator.go, line 82 at r1 (raw file):

	unaryComplementOp

	functionOp

This is a misnomer.

Make this callOp or functionCallOp

---

pkg/sql/opt/operator.go, line 87 at r1 (raw file):

)

type operatorInfo struct {

Detail what this struct is for in comments, as well as its members.

---

pkg/sql/opt/operator.go, line 92 at r1 (raw file):

}

var operatorTab = [numOperators]operatorInfo{

Explain this in comments.

---

pkg/sql/opt/operator.go, line 103 at r1 (raw file):

}

func registerOperator(op operator, name string, class operatorClass) {

Explain what this does and why it exists.

---

pkg/sql/opt/scalar.go, line 26 at r1 (raw file):

func init() {
	registerOperator(variableOp, "variable", scalarClass{})

You could define a scalar-specific operatorTab here and have it merged into the global one. I don't think that calling registerOperator many times in the proper way to structure this code.

---

pkg/sql/opt/scalar.go, line 40 at r1 (raw file):

	registerOperator(geOp, "ge", scalarClass{})
	registerOperator(neOp, "ne", scalarClass{})
	registerOperator(inOp, "IN", scalarClass{})

the capitals here and below are inconsistent with the other operators.

This needs to either be made consistent, or extensively documented both here and the commit message to justify the inconsistency.

---

pkg/sql/opt/scalar.go, line 99 at r1 (raw file):

}

func initConstExpr(e *expr, datum tree.Datum) {

These various constructors must be commented / explained further.

---

pkg/sql/opt/tree_print.go, line 1 at r1 (raw file):

// Copyright 2017 The Cockroach Authors.

This file belongs to a new package sql/sem/expr.

---

Comments from Reviewable

[knz]

@petermattis it's very clear to me that any of Jordan, Nathan, Toby or Justin must be able to review and understand this code without looking at opttoy. If they don't (or can't) we have a serious organisatory problem.

[RaduBerinde]

I am happy to add more comments and make things more understandable (within reason).

Also it is a mistake to place the expr struct definitions in the opt package. It should go to sql/sem/expr.

I don't yet have a picture of how this should be divided into packages; I would rather keep things in one place for now and move things out later (not to mention that having expr.Expr and tree.Expr is confusing, so I'd rather not start with that). Note that as far as the initial constraint-generation project is concerned, expr can be private to opt.

[RaduBerinde]

Review status: 8 of 9 files reviewed at latest revision, 21 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/misc.go, line 19 at r1 (raw file):

Previously, knz (kena) wrote…

This function should be added in a separate commit.

Huh? These are all new files, and you chose this one-line-wrapper as something that needs its own commit?

---

pkg/sql/opt/tree_print.go, line 30 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The name treePrinter is slightly confusing given the sql/sem/tree package. Perhaps this should be exprPrinter. Or perhaps exprDebugPrinter, though that is a bit verbose.

I was thinking of moving this to util as it's not specific to expressions and could be useful for other things (like EXPLAIN).

---

Comments from Reviewable

[knz]

ok we can think about separate packages later, I agree.

[RaduBerinde]

Updated the commit message, PTAL. I will address the rest of the comments later today.

---

Review status: 8 of 9 files reviewed at latest revision, 22 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[knz]

FWIW I have read the code and I recognize in it what the RFC defines and that it aligns with the overall directions. @jordanlewis can you do quality control on it?

[RaduBerinde]

Updated. Moved tree printer in util/treeprinter and improved the interface and made it look prettier (as suggested by Peter earlier). I will update opttoy to use this as well.

---

Review status: 0 of 9 files reviewed at latest revision, 20 unresolved discussions.

---

pkg/sql/opt/build.go, line 80 at r1 (raw file):

Previously, knz (kena) wrote…

Document these two constants.

Done.

---

pkg/sql/opt/expr.go, line 18 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Either pull in some of the additional commentary from opttoty or add another TODO about expanding the comments. The opttoy comments probably need to be rewritten. I'm fine with a TODO.

Added a few more comments and TODO. A lot of the comments in opttoy pertain to things that are not here yet, like relational operators and auxiliary subexpressions. I don't want to talk about those yet, they will just be confusing for now.

---

pkg/sql/opt/expr.go, line 19 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I imagine we'll have to export expr at some point, though we can cross that bridge when we reach it.

Yeah, I don't have a clear picture yet of what will be public. It doesn't need to be public for the index constraints thing so we should be good for now.

---

pkg/sql/opt/expr.go, line 24 at r1 (raw file):

Previously, knz (kena) wrote…

Extend the comment to remind the reader what are scalar properties.

Done, and added a comment for the scalarProps type.

---

pkg/sql/opt/operator.go, line 26 at r1 (raw file):

Previously, knz (kena) wrote…

Every operator below exists is rather self-explanatory. The first 4 are non-trivial however and should be further explained in comments.

Done.

---

pkg/sql/opt/operator.go, line 82 at r1 (raw file):

Previously, knz (kena) wrote…

This is a misnomer.

Make this callOp or functionCallOp

Done.

---

pkg/sql/opt/operator.go, line 87 at r1 (raw file):

Previously, knz (kena) wrote…

Detail what this struct is for in comments, as well as its members.

Done.

---

pkg/sql/opt/operator.go, line 92 at r1 (raw file):

Previously, knz (kena) wrote…

Explain this in comments.

Done.

---

pkg/sql/opt/operator.go, line 103 at r1 (raw file):

Previously, knz (kena) wrote…

Explain what this does and why it exists.

Done.

---

pkg/sql/opt/opt_test.go, line 55 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Time to copy over the help text from rewrite-results-in-testfiles. I was lazy in not doing that before.

Done.

---

pkg/sql/opt/scalar.go, line 26 at r1 (raw file):

Previously, knz (kena) wrote…

You could define a scalar-specific operatorTab here and have it merged into the global one. I don't think that calling registerOperator many times in the proper way to structure this code.

Done.

---

pkg/sql/opt/scalar.go, line 40 at r1 (raw file):

Previously, knz (kena) wrote…

the capitals here and below are inconsistent with the other operators.

This needs to either be made consistent, or extensively documented both here and the commit message to justify the inconsistency.

I don't know any reason for this, made them all lowerase.

---

pkg/sql/opt/scalar.go, line 99 at r1 (raw file):

Previously, knz (kena) wrote…

These various constructors must be commented / explained further.

Done.

---

pkg/sql/opt/testdata/build-scalar, line 11 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Are the 2 blank lines intentional? If yes, you're not consistent below.

Fixed.

---

pkg/sql/opt/tree_print.go, line 34 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/adedd/added/g

Done.

---

Comments from Reviewable

[petermattis]

Review status: 0 of 9 files reviewed at latest revision, 22 unresolved discussions.

---

pkg/sql/opt/build.go, line 145 at r3 (raw file):

	case *tree.FuncExpr:
		def, err := t.Func.Resolve(tree.SearchPath{})

Isn't the function already resolved by this point? I would have imagined we would need to resolve the function before typing. Answering my own question: looks like this is just the method you call to get the resolved function. I suppose we could add a ResolvedFunc method (similar to ResolvedType) for clarity, but doesn't seem too important.

---

pkg/sql/opt/scalar.go, line 86 at r3 (raw file):

// scalarProps are properties specific to scalar operators.
type scalarProps struct {
	typ types.T

Let's comment this field as long as you're on a commenting spree.

---

pkg/sql/opt/scalar.go, line 105 at r3 (raw file):

// The following initializers are called on an already allocated expression
// node. The scalarProps are be initialized separately.

s/are be/will be/g

---

pkg/sql/opt/testdata/build-scalar, line 45 at r3 (raw file):

      │    │    │    │    ├── const (5) (type: int)
      │    │    ├──  │ (t │e: bool)
      │    │    │    └──  │puts

Something seems messed up here. The about should read inputs. And the previous line also seems to have a problem where we have t │e instead of type.

---

pkg/sql/opt/testdata/build-scalar, line 51 at r3 (raw file):

           └──  │puts
                ├── variable (1) (type: int)
                └── const (4) (type: int)

Very pretty!

---

Comments from Reviewable

[RaduBerinde]

@jordanlewis can you do quality control on it?

@jordanlewis To expand on this request - it would be useful to get your review as someone who hasn't looked at opttoy in detail. There may be things which seem "obvious" to us but are not. There is no rush though. Thanks!

[RaduBerinde]

Review status: 0 of 9 files reviewed at latest revision, 22 unresolved discussions, some commit checks pending.

---

pkg/sql/opt/build.go, line 145 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Isn't the function already resolved by this point? I would have imagined we would need to resolve the function before typing. Answering my own question: looks like this is just the method you call to get the resolved function. I suppose we could add a ResolvedFunc method (similar to ResolvedType) for clarity, but doesn't seem too important.

We don't hold on to the FunctionDefinition in FuncExpr so I don't know how to return one without resolving it again.

---

pkg/sql/opt/scalar.go, line 86 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Let's comment this field as long as you're on a commenting spree.

Done.

---

pkg/sql/opt/scalar.go, line 105 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/are be/will be/g

Done.

---

pkg/sql/opt/testdata/build-scalar, line 45 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Something seems messed up here. The about should read inputs. And the previous line also seems to have a problem where we have t │e instead of type.

Oh, this is bad, thank you for noticing! Fixed (and improved the treeprinter test).

---

Comments from Reviewable

[petermattis]

Review status: 0 of 9 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

pkg/sql/opt/build.go, line 145 at r3 (raw file):

Previously, RaduBerinde wrote…

We don't hold on to the FunctionDefinition in FuncExpr so I don't know how to return one without resolving it again.

We do hold onto the FunctionDefinition. tree.ResolvableFunctionReference.Resolve() caches the function resolution. My point was that is is somewhat confusing that FuncExpr.Resolve is using this cached data and perhaps we should add a FuncExpr.ResolvedFunc that requires the function to already be resolved.

---

Comments from Reviewable

[RaduBerinde]

Review status: 0 of 10 files reviewed at latest revision, 17 unresolved discussions.

---

pkg/sql/opt/build.go, line 145 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

We do hold onto the FunctionDefinition. tree.ResolvableFunctionReference.Resolve() caches the function resolution. My point was that is is somewhat confusing that FuncExpr.Resolve is using this cached data and perhaps we should add a FuncExpr.ResolvedFunc that requires the function to already be resolved.

Ah, got it. Done, PTAL

---

Comments from Reviewable

[jordanlewis]

@RaduBerinde @knz I am planning to review this soon!

[rytaft]

Reviewed 2 of 9 files at r4, 7 of 8 files at r5.
Review status: 9 of 10 files reviewed at latest revision, 26 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/build.go, line 76 at r5 (raw file):

// We allocate *scalarProps and *expr in chunks of these sizes.
const exprAllocChunk = 16
const scalarPropsAllocChunk = 16

Where did 16 come from? Why is that a good choice?

---

pkg/sql/opt/expr.go, line 61 at r5 (raw file):

}

// format is part of the operatorInfo interface.

Don't you mean operatorClass interface?

---

pkg/sql/opt/misc.go, line 19 at r5 (raw file):

import "fmt"

func panicf(format string, args ...interface{}) {

This seems like a basic function that would be useful for other packages too. Maybe it belongs in util? (Maybe this was Raphael's intention in asking for a separate commit?)

Anyway, misc.go isn't a very informative name....

---

pkg/sql/opt/opt_test.go, line 18 at r5 (raw file):

// This file is home to TestOpt, which is similar to the logic tests, except it
// is used for optimizer-specific testcases.

Why not just augment logic_test.go with the build-scalar command? It seems like a lot of this code is repeated from that file.

---

pkg/sql/opt/opt_test.go, line 85 at r5 (raw file):

type testdata struct {
	pos      string // file and line number
	cmd      string // exec, query, ...

I thought currently only build-scalar was supported (this comment should be fixed)

---

pkg/sql/opt/scalar.go, line 115 at r5 (raw file):

}

// initFunctionCallExpra initializes a functionCallOp expression node.

initFunctionCallExpra -> initFunctionCallExpr

---

pkg/sql/opt/scalar.go, line 135 at r5 (raw file):

// initVariableExpr initializes a variableOp expression node.
// TODO(radu): currently variableOp refers to a column index (derived from an

What should it refer to instead?

---

pkg/sql/opt/testdata/build-scalar, line 51 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Very pretty!

+1

---

pkg/sql/opt/testdata/build-scalar, line 6 at r5 (raw file):

const (1) (type: int)

build-scalar

Why is this test case repeated?

---

pkg/util/treeprinter/tree_printer.go, line 52 at r5 (raw file):

//    ├── child-2
//    │    └── grandchild
//    └── child-1

child-1 -> child-3

---

pkg/util/treeprinter/tree_printer.go, line 114 at r5 (raw file):

	n.tree.rows = append(n.tree.rows, row)

	// Return a TreePrinter that can be used for children of this nodes.

nodes -> node

---

Comments from Reviewable

[RaduBerinde]

TFTR!

---

Review status: 1 of 9 files reviewed at latest revision, 26 unresolved discussions.

---

pkg/sql/opt/build.go, line 76 at r5 (raw file):

Previously, rytaft wrote…

Where did 16 come from? Why is that a good choice?

It's an arbitrary choice, just amortizing some allocations.

---

pkg/sql/opt/expr.go, line 61 at r5 (raw file):

Previously, rytaft wrote…

Don't you mean operatorClass interface?

Done.

---

pkg/sql/opt/opt_test.go, line 18 at r5 (raw file):

Previously, rytaft wrote…

Why not just augment logic_test.go with the build-scalar command? It seems like a lot of this code is repeated from that file.

First, logic test is in a separate package and this test should be able to use unexported opt functionality. Second, logic-test always runs SQL, not clear adding this kind of functionality there is a good idea.

---

pkg/sql/opt/opt_test.go, line 85 at r5 (raw file):

Previously, rytaft wrote…

I thought currently only build-scalar was supported (this comment should be fixed)

Removed the comment (the supported commands are described in the beginning).

---

pkg/sql/opt/scalar.go, line 115 at r5 (raw file):

Previously, rytaft wrote…

initFunctionCallExpra -> initFunctionCallExpr

Done.

---

pkg/sql/opt/scalar.go, line 135 at r5 (raw file):

Previously, rytaft wrote…

What should it refer to instead?

Removed the TODO

---

pkg/sql/opt/testdata/build-scalar, line 6 at r5 (raw file):

Previously, rytaft wrote…

Why is this test case repeated?

Fixed

---

pkg/util/treeprinter/tree_printer.go, line 52 at r5 (raw file):

Previously, rytaft wrote…

child-1 -> child-3

Done.

---

pkg/util/treeprinter/tree_printer.go, line 114 at r5 (raw file):

Previously, rytaft wrote…

nodes -> node

Done.

---

pkg/sql/opt/misc.go, line 19 at r5 (raw file):

Previously, rytaft wrote…

This seems like a basic function that would be useful for other packages too. Maybe it belongs in util? (Maybe this was Raphael's intention in asking for a separate commit?)

Anyway, misc.go isn't a very informative name....

Sigh.. removed it.

---

Comments from Reviewable

[rytaft]

---

Reviewed 2 of 9 files at r6, 7 of 7 files at r7.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks pending.

---

pkg/sql/opt/scalar.go, line 135 at r5 (raw file):

Previously, RaduBerinde wrote…

Removed the TODO

It would help to keep the info about what index means, though.

---

pkg/util/treeprinter/tree_printer.go, line 52 at r5 (raw file):

Previously, RaduBerinde wrote…

Done.

you changed it to child-4 instead of child-3 (at least make it consistent with the code above)

---

Comments from Reviewable

[RaduBerinde]

Review status: 1 of 9 files reviewed at latest revision, 18 unresolved discussions.

---

pkg/sql/opt/scalar.go, line 135 at r5 (raw file):

Previously, rytaft wrote…

It would help to keep the info about what index means, though.

Done.

---

pkg/util/treeprinter/tree_printer.go, line 52 at r5 (raw file):

Previously, rytaft wrote…

you changed it to child-4 instead of child-3 (at least make it consistent with the code above)

Sorry, fixed.

---

Comments from Reviewable

[rytaft]

Reviewed 1 of 8 files at r8, 7 of 7 files at r9.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[jordanlewis]

---

Reviewed 1 of 9 files at r4, 8 of 8 files at r8, 7 of 7 files at r9.
Review status: all files reviewed at latest revision, 21 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/expr.go, line 20 at r9 (raw file):

// expr implements the node of a unified expressions tree for both relational
// and scalar expressions in a query.

I would benefit from examples here as an outsider. I imagine a scalar expression is what our current "expression tree" represents. What is a relational expression precisely? Does it correspond to what we call a "statement" or is it more limited than that? Can you give some examples?

I see below that you're planning to do that later - that's okay then.

---

pkg/sql/opt/expr.go, line 22 at r9 (raw file):

// and scalar expressions in a query.
//
// Expressions have optional inputs. Expressions also maintain properties. For

Also would be nice to have examples here -of course they maintain properties. Nearly everything in software does! What is an expression property?

---

pkg/sql/opt/expr.go, line 37 at r9 (raw file):

	// Scalar properties (properties that pertain only to scalar operators).
	scalarProps *scalarProps
	// Operator-dependent data used by this expression. For example, constOp

This is good - could you add a For example line to the other fields?

When code gets this generic, I think it's very helpful to provide copious examples.

---

pkg/sql/opt/operator.go, line 75 at r9 (raw file):

	allOp

	bitandOp

nit: For consistency with the camel cased ones above consider bitAndOp etc

---

pkg/sql/opt/operator.go, line 95 at r9 (raw file):

	functionCallOp

	numOperators

What is numOperators?

Oh now I see it is around to get the number of operators! Please add a comment that this must come last.

---

Comments from Reviewable

[RaduBerinde]

TFTRs!

---

Review status: all files reviewed at latest revision, 21 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/expr.go, line 20 at r9 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I would benefit from examples here as an outsider. I imagine a scalar expression is what our current "expression tree" represents. What is a relational expression precisely? Does it correspond to what we call a "statement" or is it more limited than that? Can you give some examples?

I see below that you're planning to do that later - that's okay then.

Yeah, I will add more information when we support relational expressions. I figured it'd be confusing to explain things that aren't implemented.

---

pkg/sql/opt/expr.go, line 22 at r9 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Also would be nice to have examples here -of course they maintain properties. Nearly everything in software does! What is an expression property?

Fixed the comment a bit, PTAL. I'm not sure what more to say. There will be more information when we support more kinds of expressions.

---

pkg/sql/opt/expr.go, line 37 at r9 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This is good - could you add a For example line to the other fields?

When code gets this generic, I think it's very helpful to provide copious examples.

Added a line for children. The scalarProps type is pretty well defined.

---

pkg/sql/opt/operator.go, line 75 at r9 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: For consistency with the camel cased ones above consider bitAndOp etc

This is consistent with tree.Bitand etc. I guess we treat them as one word.

---

pkg/sql/opt/operator.go, line 95 at r9 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

What is numOperators?

Oh now I see it is around to get the number of operators! Please add a comment that this must come last.

Done. This is a very common pattern.

---

Comments from Reviewable