-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: support lookup join on secondary index #25628
Conversation
I'm also planning to add tests to |
Is it acceptable not to add a new version for this feature since lookup joins are still experimental? Review status: 0 of 3 files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
Nice, this looks easier than I though it would be! I would add a version, but I wouldn't bump the min-accepted version. Review status: 0 of 3 files reviewed at latest revision, all discussions resolved. pkg/sql/distsqlrun/joinreader.go, line 168 at r1 (raw file):
I don't understand what we're doing here. Aren't Also, don't mutate the slice that belongs to pkg/sql/distsqlrun/joinreader.go, line 171 at r1 (raw file):
return pkg/sql/distsqlrun/joinreader.go, line 179 at r1 (raw file):
[nit] indices (we use "indexes" for table indexes) pkg/sql/distsqlrun/joinreader.go, line 182 at r1 (raw file):
Same problem here, you are mapping Column IDs to ordinals. This perhaps happens to work because the table you are testing with didn't have schema changes, but won't work in general. Comments from Reviewable |
Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions. pkg/sql/distsqlrun/joinreader.go, line 182 at r1 (raw file): Previously, RaduBerinde wrote…
Got it, thank you for clarifying that. What is the preferred way to map Column ID to ordinal? Iterate over Comments from Reviewable |
Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions. pkg/sql/distsqlrun/joinreader.go, line 182 at r1 (raw file): Previously, solongordon (Solon) wrote…
Comments from Reviewable |
79776af
to
fc6f5da
Compare
Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions. pkg/sql/distsql_physical_planner.go, line 802 at r1 (raw file):
I think you probably should be using the index ID (i.e. pkg/sql/distsql_physical_planner.go, line 1831 at r1 (raw file):
Do we need to check for the inverse scenario, where the right side of the join can be used to "lookup" in the left side? Or is that already checked for via some other means? @RaduBerinde? pkg/sql/distsqlrun/joinreader.go, line 65 at r1 (raw file):
Comments from Reviewable |
fc6f5da
to
6e0769f
Compare
Review status: 0 of 9 files reviewed at latest revision, 7 unresolved discussions. pkg/sql/distsql_physical_planner.go, line 802 at r1 (raw file): Previously, petermattis (Peter Mattis) wrote…
Hm, this was just a refactor so I could pull out logic for getting an pkg/sql/distsqlrun/joinreader.go, line 65 at r1 (raw file): Previously, petermattis (Peter Mattis) wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 168 at r1 (raw file): Previously, RaduBerinde wrote…
Done. I was fuzzy on the ordinal vs ID distinction but now I've seen the light. pkg/sql/distsqlrun/joinreader.go, line 171 at r1 (raw file): Previously, RaduBerinde wrote…
Done. I had to add in the input cols to pkg/sql/distsqlrun/joinreader.go, line 179 at r1 (raw file): Previously, RaduBerinde wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 182 at r1 (raw file): Previously, RaduBerinde wrote…
Done. Comments from Reviewable |
pkg/sql/distsql_physical_planner.go, line 802 at r1 (raw file): Previously, solongordon (Solon) wrote…
We are putting whatever version of Comments from Reviewable |
6e0769f
to
27608f8
Compare
LGTM modulo some comments. Still need to add a version. Reviewed 1 of 7 files at r2. pkg/sql/distsql_physical_planner.go, line 1831 at r1 (raw file): Previously, petermattis (Peter Mattis) wrote…
I don't have all the context here, but perhaps the idea was that the query can be rewritten to flip the order if necessary. If we want to handle the symmetric case, it should be a separate PR. pkg/sql/distsqlrun/joinreader.go, line 64 at r2 (raw file):
This block could use a comment pkg/sql/distsqlrun/joinreader.go, line 125 at r2 (raw file):
We are calculating this map multiple times (once here, and once in every call to initRowFetcher). We should just calculate it once and pass it initRowFetcher. pkg/sql/distsqlrun/joinreader.go, line 127 at r2 (raw file):
Add a comment for which it's clear how pkg/sql/distsqlrun/joinreader.go, line 181 at r2 (raw file):
What about StoreColumnIDs? pkg/sql/distsqlrun/joinreader.go, line 184 at r2 (raw file):
maybe getIndexColSet pkg/sql/distsqlrun/joinreader.go, line 188 at r2 (raw file):
What about ExtraColumnIDs, StoredColumnIDs? pkg/sql/logictest/testdata/logic_test/lookup_join, line 275 at r2 (raw file):
These tests don't verify that what the processor is doing internally is right. For example, it might be doing secondary lookups unnecessarily. We should add a group of tests that use the KV TRACING facility to make sure we are doing the correct lookups (like https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logictest/testdata/logic_test/insert#L46). The tests should contain cases that break if we don't consider ExtraColumnIDs and StoreColumnIDs. Comments from Reviewable |
Review status: 1 of 9 files reviewed at latest revision, 9 unresolved discussions. pkg/sql/distsql_physical_planner.go, line 802 at r1 (raw file): Previously, RaduBerinde wrote…
Ah, got it. Agreed that this is safe. Carry on. pkg/sql/distsql_physical_planner.go, line 1831 at r1 (raw file): Previously, RaduBerinde wrote…
Do we do any join associativity in the 2.0 planner? If not, we might want to document this restriction when we add documentation for Comments from Reviewable |
Review status: 1 of 9 files reviewed at latest revision, 9 unresolved discussions. pkg/sql/distsql_physical_planner.go, line 1831 at r1 (raw file): Previously, petermattis (Peter Mattis) wrote…
We don't. Agreed on the documentation. Comments from Reviewable |
Review status: 1 of 9 files reviewed at latest revision, 9 unresolved discussions. pkg/sql/distsql_physical_planner.go, line 1831 at r1 (raw file): Previously, RaduBerinde wrote…
Cc @lhirata who I believe is documenting Comments from Reviewable |
Thanks for taking this on, left some minor comments Review status: 1 of 9 files reviewed at latest revision, 9 unresolved discussions. pkg/sql/distsqlrun/joinreader.go, line 181 at r2 (raw file): Previously, RaduBerinde wrote…
This looks sort of similar to what is in the pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file):
This might benefit from using a scratch slice instead. pkg/sql/distsqlrun/joinreader.go, line 453 at r2 (raw file):
Add comments to arguments pkg/sql/distsqlrun/joinreader.go, line 466 at r2 (raw file):
I'm not sure how necessary this assertion is. If you want to keep it, you might want to consider exposing pkg/sql/logictest/testdata/logic_test/lookup_join, line 275 at r2 (raw file): Previously, RaduBerinde wrote…
I don't think we plumb kv tracing down to distsql, this might need to be done in a separate PR. Comments from Reviewable |
27608f8
to
d7837d2
Compare
Thanks all for the comments. I think I've addressed everything except for adding more granular unit testing. I also still have a TODO for batching the primary index fetches, which I'd like to do in a follow-up PR if no one objects. Review status: 0 of 12 files reviewed at latest revision, 12 unresolved discussions. pkg/sql/distsqlrun/joinreader.go, line 64 at r2 (raw file): Previously, RaduBerinde wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 125 at r2 (raw file): Previously, RaduBerinde wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 127 at r2 (raw file): Previously, RaduBerinde wrote…
Done, hopefully clearer now. I renamed pkg/sql/distsqlrun/joinreader.go, line 181 at r2 (raw file): Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Good call, done. pkg/sql/distsqlrun/joinreader.go, line 184 at r2 (raw file): Previously, RaduBerinde wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 188 at r2 (raw file): Previously, RaduBerinde wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Could you clarify? Unless I'm misunderstanding I think this is already a scratch slice. ( pkg/sql/distsqlrun/joinreader.go, line 453 at r2 (raw file): Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Done. pkg/sql/distsqlrun/joinreader.go, line 466 at r2 (raw file): Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Yeah it's not strictly necessary, but it's cheap and if it fails we're probably returning incorrect results, so I thought it was worth it. pkg/sql/logictest/testdata/logic_test/lookup_join, line 275 at r2 (raw file): Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Maybe this can be addressed in Comments from Reviewable |
Review status: 0 of 12 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, solongordon (Solon) wrote…
On every lookup, you're creating a new cockroach/pkg/sql/distsqlrun/aggregator.go Line 726 in cc4511e
Comments from Reviewable |
Review status: 0 of 12 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Ah got it, thanks. If it's cool with you I think I'll hold off on the scratch slice for now since this should be a small array which is allocated on the stack. Also this code is likely to change when I add batching. pkg/sql/logictest/testdata/logic_test/lookup_join, line 275 at r2 (raw file): Previously, solongordon (Solon) wrote…
I added a unit test which covers the various cases here (ColumnID, ExtraColumnID, StoreColumnID, and non-covering index.) Comments from Reviewable |
d7837d2
to
b66f773
Compare
d8230cf
to
39d7192
Compare
joinReader now supports lookup joins on secondary indexes. This was a trivial change for queries where all the output columns are included in the secondary index. I just modified the physical planner to specify the secondary index in the JoinReaderSpec and removed checks which prevented secondary indexes from being used. The more complicated situation is when we want to do a lookup join against a non-covering index. In this case, the logical planner plans an index join before the inner join, but we want to perform the lookup join first. We now handle this by only planning the lookup join during physical planning, not the index join. During execution, the joinReader detects that there are output columns not covered by the secondary index, and it performs primary index lookups as necessary to retrieve the additional columns. Fixes cockroachdb#25431 Release note (sql change): The experimental lookup join feature now supports secondary indexes.
39d7192
to
36c1f98
Compare
Review status: 0 of 16 files reviewed at latest revision, 7 unresolved discussions. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, solongordon (Solon) wrote…
Ok to hold off, but wanted to mention that Comments from Reviewable |
Review status: 0 of 16 files reviewed at latest revision, 7 unresolved discussions. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, RaduBerinde wrote…
Hm, I came across this thread which seems to indicate otherwise, but I'd love to know more. https://groups.google.com/d/msg/golang-nuts/KdbtOqNK6JQ/ehUOmI7LaKwJ Comments from Reviewable |
Review status: 0 of 16 files reviewed at latest revision, 7 unresolved discussions. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, solongordon (Solon) wrote…
My understanding is that Dave Cheney is wrong there and that Comments from Reviewable |
bors r+ |
25005: ui: add top-level LoginContainer: require login before rendering anything r=couchand a=vilterp Depends on #25057 Touches #24939 ![](https://user-images.githubusercontent.com/7341/39150096-38b4cd28-470f-11e8-9d67-e1832d35a211.gif) Shown above: 1. go to admin UI; see login screen 2. error message when you type in the wrong password 3. you can't hit an authenticated endpoint because you don't have a valid session (this checking is turned on by #24944, only in secure mode) 4. once you login with the right password, you can see the UI (the temporary "connection lost" banner shouldn't be there) 5. now you (and the UI itself) can hit endpoints, because you have a valid session Todos: - [ ] redirect to login page instead of current wrapper component switching thing - [ ] logout - [ ] logout button (make API call; redirect to login & blow away redux store) - [ ] log out other tabs (if they get a 403, redirect to login & blow away redux store) - [ ] styling Release note: None 25628: distsql: support lookup join on secondary index r=solongordon a=solongordon joinReader now supports lookup joins on secondary indexes. This was a trivial change for queries where all the output columns are included in the secondary index. I just modified the physical planner to specify the secondary index in the JoinReaderSpec and removed checks which prevented secondary indexes from being used. The more complicated situation is when we want to do a lookup join against a non-covering index. In this case, the logical planner plans an index join before the inner join, but we want to perform the lookup join first. We now handle this by only planning the lookup join during physical planning, not the index join. During execution, the joinReader detects that there are output columns not covered by the secondary index, and it performs primary index lookups as necessary to retrieve the additional columns. Fixes #25431 Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com> Co-authored-by: Andrew Couch <andrew@cockroachlabs.com> Co-authored-by: Solon Gordon <solon@cockroachlabs.com>
Build succeeded |
Review status: 0 of 16 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, petermattis (Peter Mattis) wrote…
I did an experiment which appears to back up Dave. As far as I can tell the slice is allocated on the heap when it exceeds 8192 bytes, which I suppose is the stack frame size.
Comments from Reviewable |
TIL! |
Review status: 0 of 16 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, solongordon (Solon) wrote…
The plot thickens though: the compiler isn't smart enough to allocate on the stack if the length is not a constant. I think that means it will allocate on the heap in this case after all. Comments from Reviewable |
Review status: 0 of 16 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful. pkg/sql/distsqlrun/joinreader.go, line 443 at r2 (raw file): Previously, solongordon (Solon) wrote…
Thanks for following up on this. TIL that the compiler will allocate constant length slices on the stack in some instances. That's interesting. Comments from Reviewable |
joinReader now supports lookup joins on secondary indexes. This was a
trivial change for queries where all the output columns are included in
the secondary index. I just modified the physical planner to specify the
secondary index in the JoinReaderSpec and removed checks which prevented
secondary indexes from being used.
The more complicated situation is when we want to do a lookup join
against a non-covering index. In this case, the logical planner plans an
index join before the inner join, but we want to perform the lookup join
first. We now handle this by only planning the lookup join during
physical planning, not the index join. During execution, the joinReader
detects that there are output columns not covered by the secondary
index, and it performs primary index lookups as necessary to retrieve
the additional columns.
Fixes #25431