sentry: comparison ops with suboperators don't handle NULLs

[cockroach-teamcity]

This issue was autofiled by Sentry. It represents a crash or reported error on a live cluster with telemetry enabled.

Sentry link: https://sentry.io/organizations/cockroach-labs/issues/1029811314/?referrer=webhooks_plugin

Panic message:

(0) eval.go:3600: unhandled right expression %s | tree.dNull
(see stack traces in additional data)

Stacktrace (expand for inline code snippets):

cockroach/pkg/sql/sem/tree/eval.go

Lines 3599 to 3601 in 25dd36f

	} else {
	return nil, pgerror.NewAssertionErrorf("unhandled right expression %s", right)
	}

in pkg/sql/sem/tree.(*ComparisonExpr).Eval

cockroach/pkg/sql/sem/tree/eval.go

Lines 2780 to 2782 in 25dd36f

	}
	right, err := expr.Right.(TypedExpr).Eval(ctx)
	if err != nil {

in pkg/sql/sem/tree.(*AndExpr).Eval

cockroach/pkg/sql/sqlbase/expr_filter.go

Lines 25 to 27 in 25dd36f

	d, err := filter.Eval(evalCtx)
	if err != nil {

in pkg/sql/sqlbase.RunFilter

cockroach/pkg/sql/distsqlrun/expr.go

Lines 178 to 180 in 25dd36f

	eh.evalCtx.PushIVarContainer(eh)
	pass, err := sqlbase.RunFilter(eh.expr, eh.evalCtx)
	eh.evalCtx.PopIVarContainer()

in pkg/sql/distsqlrun.(*exprHelper).evalFilter

cockroach/pkg/sql/distsqlrun/processors.go

Lines 354 to 356 in 25dd36f

	// Filtering.
	passes, err := h.filter.evalFilter(row)
	if err != nil {

in pkg/sql/distsqlrun.(*ProcOutputHelper).ProcessRow

cockroach/pkg/sql/distsqlrun/processors.go

Lines 777 to 779 in 25dd36f

	func (pb *ProcessorBase) ProcessRowHelper(row sqlbase.EncDatumRow) sqlbase.EncDatumRow {
	outRow, ok, err := pb.out.ProcessRow(pb.Ctx, row)
	if err != nil {

in pkg/sql/distsqlrun.(*ProcessorBase).ProcessRowHelper

cockroach/pkg/sql/distsqlrun/tablereader.go

Lines 289 to 291 in 25dd36f

	if outRow := tr.ProcessRowHelper(row); outRow != nil {
	return outRow, nil

in pkg/sql/distsqlrun.(*tableReader).Next

cockroach/pkg/sql/distsqlrun/aggregator.go

Lines 532 to 534 in 25dd36f

	for {
	row, meta := ag.input.Next()
	if meta != nil {

in pkg/sql/distsqlrun.(*orderedAggregator).accumulateRows

cockroach/pkg/sql/distsqlrun/aggregator.go

Lines 720 to 722 in 25dd36f

	case aggAccumulating:
	ag.runningState, row, meta = ag.accumulateRows()
	case aggEmittingRows:

in pkg/sql/distsqlrun.(*orderedAggregator).Next

cockroach/pkg/sql/distsqlrun/base.go

Lines 173 to 175 in 25dd36f

	for {
	row, meta := src.Next()
	// Emit the row; stop if no more rows are needed.

in pkg/sql/distsqlrun.Run

cockroach/pkg/sql/distsqlrun/processors.go

Lines 800 to 802 in 25dd36f

	ctx = pb.self.Start(ctx)
	Run(ctx, pb.self, pb.out.output)
	}

in pkg/sql/distsqlrun.(*ProcessorBase).Run

cockroach/pkg/sql/distsqlrun/flow.go

Lines 625 to 627 in 25dd36f

	}
	headProc.Run(ctx)
	return nil

in pkg/sql/distsqlrun.(*Flow).Run

cockroach/pkg/sql/distsql_running.go

Lines 251 to 253 in 25dd36f

	// TODO(radu): this should go through the flow scheduler.
	if err := flow.Run(ctx, func() {}); err != nil {
	log.Fatalf(ctx, "unexpected error from syncFlow.Start(): %s "+

in pkg/sql.(*DistSQLPlanner).Run

cockroach/pkg/sql/distsql_running.go

Lines 838 to 840 in 25dd36f

	dsp.FinalizePlan(planCtx, &physPlan)
	dsp.Run(planCtx, txn, &physPlan, recv, evalCtx, nil /* finishedSetupFn */)
	}

in pkg/sql.(*DistSQLPlanner).PlanAndRun

cockroach/pkg/sql/conn_executor_exec.go

Lines 1111 to 1113 in 25dd36f

	// the planner whether or not to plan remote table readers.
	ex.server.cfg.DistSQLPlanner.PlanAndRun(
	ctx, evalCtx, planCtx, planner.txn, planner.curPlan.plan, recv)

in pkg/sql.(*connExecutor).execWithDistSQLEngine

cockroach/pkg/sql/conn_executor_exec.go

Lines 947 to 949 in 25dd36f

	ex.sessionTracing.TraceExecStart(ctx, "distributed")
	err = ex.execWithDistSQLEngine(ctx, planner, stmt.AST.StatementType(), res, distributePlan)
	ex.sessionTracing.TraceExecEnd(ctx, res.Err(), res.RowsAffected())

in pkg/sql.(*connExecutor).dispatchToExecutionEngine

cockroach/pkg/sql/conn_executor_exec.go

Lines 455 to 457 in 25dd36f

	p.autoCommit = os.ImplicitTxn.Get() && !ex.server.cfg.TestingKnobs.DisableAutoCommit
	if err := ex.dispatchToExecutionEngine(ctx, p, res); err != nil {
	return nil, nil, err

in pkg/sql.(*connExecutor).execStmtInOpenState

cockroach/pkg/sql/conn_executor_exec.go

Lines 101 to 103 in 25dd36f

	} else {
	ev, payload, err = ex.execStmtInOpenState(ctx, stmt, pinfo, res)
	}

in pkg/sql.(*connExecutor).execStmt

cockroach/pkg/sql/conn_executor.go

Lines 1240 to 1242 in 25dd36f

	ctx := withStatement(ex.Ctx(), ex.curStmt)
	ev, payload, err = ex.execStmt(ctx, curStmt, stmtRes, pinfo)
	if err != nil {

in pkg/sql.(*connExecutor).run

cockroach/pkg/sql/conn_executor.go

Lines 432 to 434 in 25dd36f

	}()
	return h.ex.run(ctx, s.pool, reserved, cancel)
	}

in pkg/sql.(*Server).ServeConn

cockroach/pkg/sql/pgwire/conn.go

Lines 336 to 338 in 25dd36f

	}()
	writerErr = sqlServer.ServeConn(ctx, connHandler, reserved, cancelConn)
	// TODO(andrei): Should we sometimes transmit the writerErr's to the

in pkg/sql/pgwire.(*conn).serveImpl.func4

pkg/sql/sem/tree/eval.go in pkg/sql/sem/tree.(*ComparisonExpr).Eval at line 3600
pkg/sql/sem/tree/eval.go in pkg/sql/sem/tree.(*AndExpr).Eval at line 2781
pkg/sql/sqlbase/expr_filter.go in pkg/sql/sqlbase.RunFilter at line 26
pkg/sql/distsqlrun/expr.go in pkg/sql/distsqlrun.(*exprHelper).evalFilter at line 179
pkg/sql/distsqlrun/processors.go in pkg/sql/distsqlrun.(*ProcOutputHelper).ProcessRow at line 355
pkg/sql/distsqlrun/processors.go in pkg/sql/distsqlrun.(*ProcessorBase).ProcessRowHelper at line 778
pkg/sql/distsqlrun/tablereader.go in pkg/sql/distsqlrun.(*tableReader).Next at line 290
pkg/sql/distsqlrun/aggregator.go in pkg/sql/distsqlrun.(*orderedAggregator).accumulateRows at line 533
pkg/sql/distsqlrun/aggregator.go in pkg/sql/distsqlrun.(*orderedAggregator).Next at line 721
pkg/sql/distsqlrun/base.go in pkg/sql/distsqlrun.Run at line 174
pkg/sql/distsqlrun/processors.go in pkg/sql/distsqlrun.(*ProcessorBase).Run at line 801
pkg/sql/distsqlrun/flow.go in pkg/sql/distsqlrun.(*Flow).Run at line 626
pkg/sql/distsql_running.go in pkg/sql.(*DistSQLPlanner).Run at line 252
pkg/sql/distsql_running.go in pkg/sql.(*DistSQLPlanner).PlanAndRun at line 839
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execWithDistSQLEngine at line 1112
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).dispatchToExecutionEngine at line 948
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execStmtInOpenState at line 456
pkg/sql/conn_executor_exec.go in pkg/sql.(*connExecutor).execStmt at line 102
pkg/sql/conn_executor.go in pkg/sql.(*connExecutor).run at line 1241
pkg/sql/conn_executor.go in pkg/sql.(*Server).ServeConn at line 433
pkg/sql/pgwire/conn.go in pkg/sql/pgwire.(*conn).serveImpl.func4 at line 337

Tag	Value
Cockroach Release	v19.1.0
Cockroach SHA:	25dd36f
Platform	linux amd64
Distribution	CCL
Environment	v19.1.0
Command	server
Go Version	go1.11.6
# of CPUs	6
# of Goroutines	181

[jordanlewis]

op := expr.Operator
	if op.hasSubOperator() {
		var datums Datums
		// Right is either a tuple or an array of Datums.
		if tuple, ok := AsDTuple(right); ok {
			datums = tuple.D
		} else if array, ok := AsDArray(right); ok {
			datums = array.Array
		} else {
			return nil, pgerror.NewAssertionErrorf("unhandled right expression %s", right)
		}
		return evalDatumsCmp(ctx, op, expr.SubOperator, expr.fn, left, datums)
	}

Looks like a comparison operator with a subop assumes that the right side is either an array or a tuple, and in this case, there was a different reality. What could that reality be?

[jordanlewis]

Oh, I think the message says - it's a DNull literal?

[justinj]

repro:

select x = any string_to_array(null, ' ') from (values ('foo')) v(x);

I think this might be caused by constant-folding an array-typed value to NULL? I think that sounds valid though, so I think the bug here seems to be in the code block you pasted.

[justinj]

repro that doesn't involve constant folding:

select x = any string_to_array(x, ' ') from (values (null::string)) v(x);

[jordanlewis]

I think the fix here is to make sure that comparison exprs with sub operators explicitly look for NULL, just like all other builtins.

Here's a repro without builtins:

select 1 = any null::int[];