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

sql: apply join outer cols must be typed null #37597

Merged
merged 1 commit into from May 20, 2019

Conversation

Projects
None yet
4 participants
@jordanlewis
Copy link
Member

commented May 20, 2019

This commit prevents the apply join "replace outer columns with NULL"
step from generated untyped null expressions, which can cause panics in
certain cases.

Fixes #37454.

Release note (bug fix): fix a crash in apply join

@jordanlewis jordanlewis requested review from RaduBerinde and andy-kimball May 20, 2019

@jordanlewis jordanlewis requested review from cockroachdb/sql-execution-prs as code owners May 20, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 20, 2019

This change is Reviewable

@RaduBerinde
Copy link
Member

left a comment

Usually this is done with ReType. If we add this expression, we should look at switching those callers over as well.

We should add code in the optbuilder to build TypedNullExpr (see ConstructNull) - it's strange to only allow using it "after" optimization.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jordanlewis, and @RaduBerinde)


pkg/sql/sem/tree/expr.go, line 1692 at r1 (raw file):

func (t *TypedNullExpr) Format(ctx *FmtCtx) {
	DNull.Format(ctx)

Don't we need to add the type if we're serializing?


pkg/sql/sem/tree/type_check.go, line 1143 at r1 (raw file):

func (t *TypedNullExpr) TypeCheck(ctx *SemaContext, desired *types.T) (TypedExpr, error) {
	if typ := t.typ; !(typ.Equivalent(desired) || typ.Family() == types.UnknownFamily) {

[nit] just use t.typ, it's more clear

@jordanlewis jordanlewis force-pushed the jordanlewis:typed-null branch from c8e8fdb to f7241c4 May 20, 2019

@jordanlewis jordanlewis changed the title sql: introduce a typed null wrapper to fix a bug [wip] sql: apply join outer cols must be typed null May 20, 2019

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Thanks for the pointer on ReType. I deleted the typed-null thing, which because of serialization has to basically just be a CastExpr anyway.

Cleaner now, RFAL.

@RaduBerinde
Copy link
Member

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jordanlewis)


pkg/sql/plan_node_to_row_source.go, line 57 at r2 (raw file):

	typs := make([]types.T, len(nodeColumns))
	for i := range nodeColumns {
		if nodeColumns[i].Typ.Family() == types.UnknownFamily {

Isn't this too restrictive? SELECT NULL would have a renderNode that has an unknown type.

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Yeah, you're right. I think this probably explains several outstanding bugs in sentry. If you can make valid queries with type unknown columns, then there are several spots in DistSQL that will fail.

@RaduBerinde

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I would be surprised if distsql wasn't handling that currently, seems like some very basic queries would fail.

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Yeah, maybe you're right. The error condition here is if the type is declared as unknown and then a processor sends a non-null datum. This should only be possible if something went deeply wrong. I'll remove the checks I added for now and see where that gets us.

sql: apply join outer cols must be typed null
This commit prevents the apply join "replace outer columns with NULL"
step from generated untyped null expressions, which can cause panics in
certain cases.

Release note (bug fix): fix a crash in apply join

@jordanlewis jordanlewis force-pushed the jordanlewis:typed-null branch from f7241c4 to 7750bc5 May 20, 2019

@andy-kimball
Copy link
Contributor

left a comment

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @jordanlewis)

@jordanlewis

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

bors r+

TFTRs!

craig bot pushed a commit that referenced this pull request May 20, 2019

Merge #37556 #37597
37556: workload: minor improvements to querybench and adding TPCH vec benchmark r=yuzefovich a=yuzefovich

Adds some improvements to querybench: the number of times each query
in the query file should be run can now be specified as well as
option to turn on the vectorized execution.

Also, adds TPCH benchmark on the subset of the queries that are
supported at the moment by vectorized engine.

Release note: None

37597: sql: apply join outer cols must be typed null  r=jordanlewis a=jordanlewis

This commit prevents the apply join "replace outer columns with NULL"
step from generated untyped null expressions, which can cause panics in
certain cases.

Fixes #37454.

Release note (bug fix): fix a crash in apply join

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 20, 2019

Build succeeded

@craig craig bot merged commit 7750bc5 into cockroachdb:master May 20, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@jordanlewis jordanlewis deleted the jordanlewis:typed-null branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.