Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

distsql: implement evaluator (processor core type) #9916

Merged
merged 1 commit into from
Oct 12, 2016

Conversation

irfansharif
Copy link
Contributor

@irfansharif irfansharif commented Oct 12, 2016

Implementation of the Evaluator processor core type, a fully
programmable no-grouping aggregator that runs a 'program' on each
individual row. The 'program' is a set of expressions evaluated in
order, the results of evaluating each of these expressions on the input
row form the output.

Part of #7587, pulled out of the original work in #9793 (see this and this).

CC @RaduBerinde @andreimatei


This change is Reviewable

@RaduBerinde
Copy link
Member

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


sql/distsql/evaluator.go, line 56 at r1 (raw file):

  }

  // Loop over the expressions in our expression set and extract out fully typed expressions, this

[nit] comments are usually wrapped at 80 cols


sql/distsql/evaluator.go, line 74 at r1 (raw file):

      defer wg.Done()
  }

should open a tracing span, see e.g. sorter.Run


sql/distsql/evaluator.go, line 101 at r1 (raw file):

func (ev *evaluator) next() (sqlbase.EncDatumRow, error) {
  row, err := ev.input.NextRow()

[nit] more straightforward if we just do this directly inside Run, we're duplicating the end condition checks just to separate out a couple of lines


sql/distsql/evaluator.go, line 111 at r1 (raw file):

}

func (ev *evaluator) extract(row sqlbase.EncDatumRow) (sqlbase.EncDatumRow, error) {

s/extract/eval


sql/distsql/evaluator.go, line 112 at r1 (raw file):

func (ev *evaluator) extract(row sqlbase.EncDatumRow) (sqlbase.EncDatumRow, error) {
  tuple := make(parser.DTuple, len(ev.exprs))

we should avoid allocating this for every row. we could allocate it once and reuse it. Might be easier to just do everything directly inside Run.


sql/distsql/expr.go, line 153 at r1 (raw file):

//  '$0' would return '1'
//  '$1 + 10' would return '12'
func (eh *exprHelper) extract(row sqlbase.EncDatumRow) (parser.Datum, error) {

s/extract/eval


sql/distsql/expr.go, line 156 at r1 (raw file):

  eh.row = row

  //  TODO(irfansharif): extract here is very permissive, if expr is of type *parser.FuncExpr for

[nit] wrap at 80


sql/distsql/expr.go, line 166 at r1 (raw file):

  //  NullIfExpr.
  //
  //  TODO(irfansharif): We need to determine when exactly the 'argument extraction' takes place,

This is not something this processor (or any part of the physical plan) should be worried about. It's up to the code that will build logica and physical plans to extract expressions from aggregate and configure the processors with the correct expressions.


sql/sqlbase/encoded_datum.go, line 240 at r1 (raw file):

func DTupleToEncDatumRow(tuple parser.DTuple) (EncDatumRow, error) {
  row := make(EncDatumRow, len(tuple))

We should use a EncDatumRowAlloc


Comments from Reviewable

@irfansharif
Copy link
Contributor Author

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


sql/distsql/evaluator.go, line 56 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] comments are usually wrapped at 80 cols

ah, messed up my editor configs. Done.

sql/distsql/evaluator.go, line 74 at r1 (raw file):

Previously, RaduBerinde wrote…

should open a tracing span, see e.g. sorter.Run

Done.

sql/distsql/evaluator.go, line 101 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] more straightforward if we just do this directly inside Run, we're duplicating the end condition checks just to separate out a couple of lines

agreed, done.

sql/distsql/evaluator.go, line 111 at r1 (raw file):

Previously, RaduBerinde wrote…

s/extract/eval

Done.

sql/distsql/evaluator.go, line 112 at r1 (raw file):

Previously, RaduBerinde wrote…

we should avoid allocating this for every row. we could allocate it once and reuse it. Might be easier to just do everything directly inside Run.

I've still kept this `extract/eval` separately but changed so as to not reallocate per row. no strong opinions here, will pull it into `Run` if you still don't feel it adds to much clarity.

sql/distsql/expr.go, line 153 at r1 (raw file):

Previously, RaduBerinde wrote…

s/extract/eval

Done.

sql/distsql/expr.go, line 156 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] wrap at 80

Done.

sql/distsql/expr.go, line 166 at r1 (raw file):

Previously, RaduBerinde wrote…

This is not something this processor (or any part of the physical plan) should be worried about. It's up to the code that will build logica and physical plans to extract expressions from aggregate and configure the processors with the correct expressions.

Cool, wasn't sure if it would come here or outside when setting up the flow. thanks for clarifying.

sql/sqlbase/encoded_datum.go, line 240 at r1 (raw file):

Previously, RaduBerinde wrote…

We should use a EncDatumRowAlloc

Done.

Comments from Reviewable

@RaduBerinde
Copy link
Member

:lgtm:


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


sql/distsql/evaluator.go, line 81 at r2 (raw file):

  }

  ctx, span := tracing.ChildSpan(ev.ctx, "sorter")

"evaluator"


sql/sqlbase/encoded_datum.go, line 230 at r2 (raw file):

func DatumToEncDatum(datum parser.Datum) (EncDatum, error) {
  if dType, ok := ColumnType_Kind_value[strings.ToUpper(datum.Type())]; !ok {
      return EncDatum{}, errors.Errorf("Unknown type, could not convert to EncDatum")

Include the type in the error.


Comments from Reviewable

@irfansharif irfansharif force-pushed the dist-evaluator branch 3 times, most recently from 46e4e18 to fd1ec41 Compare October 12, 2016 19:34
Implementation of the `Evaluator` processor core type, a fully
programmable no-grouping aggregator that runs a 'program' on each
individual row. The 'program' is a set of expressions evaluated in
order, the results of evaluating each of these expressions on the input
row form the output.

Part of cockroachdb#7587.
@irfansharif
Copy link
Contributor Author

TFTR! pulling this in now.


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


pkg/sql/distsql/evaluator.go, line 81 at r2 (raw file):

Previously, RaduBerinde wrote…

"evaluator"

Done.

sql/sqlbase/encoded_datum.go, line 230 at r2 (raw file):

Previously, RaduBerinde wrote…

Include the type in the error.

Done.

Comments from Reviewable

@irfansharif irfansharif changed the title implement evaluator for distributed sql distsql: implement evaluator (processor core type) Oct 12, 2016
@irfansharif irfansharif merged commit c10dfbe into cockroachdb:master Oct 12, 2016
@irfansharif irfansharif deleted the dist-evaluator branch November 18, 2019 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants