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: add support for joinReader to be the first join in paired joins #56161

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

sumeerbhola
Copy link
Collaborator

This is to allow lookup joins to be used for left outer/semi/anti
joins with non-covering indexes. Currently only semi joins for
this case can use the index (by doing two inner joins and a DistinctOn)

Paired joins with a non-covering index will be used as follows:

  • Left outer join: will become a pair of left outer lookup joins.
  • Left anti join: will be a left outer lookup join followed by
    a left anti lookup join.
  • Left semi join: will be an inner lookup join followed by a
    left semi lookup join.

This PR does not contain the optimizer changes.

Informs #55452

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: (but @yuzefovich or @asubiotto should take a look too)

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @sumeerbhola, and @yuzefovich)


pkg/sql/execinfrapb/processors_sql.proto, line 326 at r1 (raw file):

  optional bool left_join_with_paired_joiner = 14 [(gogoproto.nullable) = false];

  // OutputGroupContinuationForLeftRow indicates that this join is a first

[nit] a first -> the first


pkg/sql/rowexec/joinerbase.go, line 87 at r1 (raw file):

	// condTypes does not include the continuation column, but the slice has the
	// capacity for it.

Why do you want to add capacity for it? (Is it so the extra capacity can be used for outputTypes? If so, would be good to say that here).

Also, since you're not using the variable condRowSize, you might as well just use one variable size or rowSize.


pkg/sql/rowexec/joinreader.go, line 882 at r1 (raw file):

}

// setMatched returns the previous value of the matched field.

[nit] this comment should also describe the main purpose of the function, which is to set the matched field to true.


pkg/sql/rowexec/joinreader_strategies.go, line 394 at r1 (raw file):

	groupingState *inputBatchGroupingState

	// True when this join is the first join in paired-joins. Note that in this

[nit] most comments for data members should start with the name of the field. E.g., outputGroupContinuationForLeftRow is true when....

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @asubiotto and @sumeerbhola)


pkg/sql/rowexec/joinerbase.go, line 95 at r1 (raw file):

	if !jb.joinType.ShouldIncludeRightColsInOutput() {
		outputTypes = outputTypes[:len(leftTypes)]
	} else if outputContinuationColumn {

I think this needs to be broken up into two independent if blocks - currently, for left semi/anti we will never append a boolean if outputContinuationColumn is true.

This is to allow lookup joins to be used for left outer/semi/anti
joins with non-covering indexes. Currently only semi joins for
this case can use the index (by doing two inner joins and a DistinctOn)

Paired joins with a non-covering index will be used as follows:
- Left outer join: will become a pair of left outer lookup joins.
- Left anti join: will be a left outer lookup join followed by
  a left anti lookup join.
- Left semi join: will be an inner lookup join followed by a
  left semi lookup join.

This PR does not contain the optimizer changes.

Informs cockroachdb#55452

Release note: None
Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @asubiotto, @rytaft, and @yuzefovich)


pkg/sql/execinfrapb/processors_sql.proto, line 326 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] a first -> the first

Done.


pkg/sql/rowexec/joinerbase.go, line 87 at r1 (raw file):

Why do you want to add capacity for it? (Is it so the extra capacity can be used for outputTypes? If so, would be good to say that here).

Yes, it is for outputTypes. Added a comment.

Also, since you're not using the variable condRowSize, you might as well just use one variable size or rowSize.

Done


pkg/sql/rowexec/joinerbase.go, line 95 at r1 (raw file):

Previously, yuzefovich wrote…

I think this needs to be broken up into two independent if blocks - currently, for left semi/anti we will never append a boolean if outputContinuationColumn is true.

We only support continuation for inner and left outer joins since it does not make sense to filter out false positives without outputting the right row. I've added a code comment that says that outputContinuationCol implies jb.joinType.ShouldIncludeRightColsInOutput() -- hopefully that makes it a bit clearer.


pkg/sql/rowexec/joinreader.go, line 882 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] this comment should also describe the main purpose of the function, which is to set the matched field to true.

Done.


pkg/sql/rowexec/joinreader_strategies.go, line 394 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] most comments for data members should start with the name of the field. E.g., outputGroupContinuationForLeftRow is true when....

Done.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @asubiotto and @rytaft)


pkg/sql/rowexec/joinerbase.go, line 95 at r1 (raw file):

Previously, sumeerbhola wrote…

We only support continuation for inner and left outer joins since it does not make sense to filter out false positives without outputting the right row. I've added a code comment that says that outputContinuationCol implies jb.joinType.ShouldIncludeRightColsInOutput() -- hopefully that makes it a bit clearer.

I see, makes sense.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto)

@sumeerbhola
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 5, 2020

Build succeeded:

@craig craig bot merged commit c456295 into cockroachdb:master Nov 5, 2020
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.

4 participants