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: proto specs for join processors #10054

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Oct 18, 2016

This change is Reviewable

@danhhz
Copy link
Contributor

danhhz commented Oct 18, 2016

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


pkg/sql/distsql/processors.proto, line 167 at r1 (raw file):

  optional JoinType type = 4 [(gogoproto.nullable) = false];

  // Columns for the output stream. If the left stream has N columns and the

very small nit: I had to read this a couple times to understand it, I kept waiting for the "else" counterpart to the "if". Maybe s/If the left/Assuming the left/?


pkg/sql/distsql/processors.proto, line 173 at r1 (raw file):

}

// HashJoinerSpec is the specification for a merge join processor. The processor

hash join processor?


Comments from Reviewable

@irfansharif
Copy link
Contributor

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


pkg/sql/distsql/processors.proto, line 139 at r1 (raw file):

// MergeJoinerSpec is the specification for a merge join processor. The processor
// has two inputs and one output. The inputs have the same ordering on the

'The inputs must have' for more clarity.


pkg/sql/distsql/processors.proto, line 141 at r1 (raw file):

// has two inputs and one output. The inputs have the same ordering on the
// columns that have equality constraints. For example:
//   SELECT FROM T1 INNER JOIN T2 ON T1.C1 = T2.C5 AND T1.C2 = T2.C4

SELECT * or some target list.


pkg/sql/distsql/processors.proto, line 144 at r1 (raw file):

//
// To perform a merge join, the streams corresponding to T1 and T2 must have the
// same ordering on coluns C1, C2 and C5, C4 respectively. For example: C1+,C2-

'columns'.


pkg/sql/distsql/processors.proto, line 161 at r1 (raw file):

  // orderings). If the left stream has N columns and the right stream has M
  // columns, in this expression variables $0 to $(N-1) refer to columns of the
  // left stream and variables $N to $(N+M-1) refer to columns in the right

how are we determining what N and M are?


pkg/sql/distsql/processors.proto, line 179 at r1 (raw file):

// table. Thus, there is no guarantee on the ordering of results that stem only
// from the left input (in the case of LEFT_OUTER, FULL_OUTER). However, it is
// guaranteed that results that involve the right stream preserve the ordering;

what would this look like for LEFT JOIN (or FULL JOIN) when there are no corresponding matches on the right stream?


pkg/sql/distsql/processors.proto, line 191 at r1 (raw file):

  // orderings). If the left stream has N columns and the right stream has M
  // columns, in this expression variables $0 to $(N-1) refer to columns of the
  // left stream and variables $N to $(N+M-1) refer to columns in the right

see above, how are we going to determine what N and M are?


Comments from Reviewable

@RaduBerinde RaduBerinde force-pushed the join-proto branch 2 times, most recently from 74e3fb0 to 62e04e5 Compare October 18, 2016 15:20
@RaduBerinde
Copy link
Member Author

TFTRs, updated


Review status: 2 of 5 files reviewed at latest revision, 8 unresolved discussions.


pkg/sql/distsql/processors.proto, line 139 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

'The inputs must have' for more clarity.

Done.

pkg/sql/distsql/processors.proto, line 141 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

SELECT * or some target list.

Done.

pkg/sql/distsql/processors.proto, line 144 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

'columns'.

Done.

pkg/sql/distsql/processors.proto, line 161 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

how are we determining what N and M are?

We will know once we receive the first row from each input. If that makes the code harder we can add them to the spec at that time.

pkg/sql/distsql/processors.proto, line 167 at r1 (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

very small nit: I had to read this a couple times to understand it, I kept waiting for the "else" counterpart to the "if". Maybe s/If the left/Assuming the left/?

Done.

pkg/sql/distsql/processors.proto, line 173 at r1 (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

hash join processor?

Done.

pkg/sql/distsql/processors.proto, line 179 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

what would this look like for LEFT JOIN (or FULL JOIN) when there are no corresponding matches on the right stream?

the previous sentence says that there is no ordering guarantee for those results

Comments from Reviewable

@irfansharif
Copy link
Contributor

:lgtm:


Review status: 2 of 5 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


pkg/sql/distsql/processors.proto, line 179 at r1 (raw file):

Previously, RaduBerinde wrote…

the previous sentence says that there is no ordering guarantee for those results

_hangs head in shame_

Comments from Reviewable

@irfansharif
Copy link
Contributor

Reviewed 1 of 5 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


Comments from Reviewable

@RaduBerinde
Copy link
Member Author

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


pkg/sql/distsql/processors.proto, line 179 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

hangs head in shame

Haha, no, it's pretty hard to explain and I don't think I did a particularly good job :)

Comments from Reviewable

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.

3 participants