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: use IndexFetchSpec in JoinReader #76963

Merged
merged 4 commits into from
Feb 25, 2022

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Feb 24, 2022

descpb: add IndexID to IndexFetchSpec

Release note: None

span: make Builder and Splitter without descriptors

Allow creation of span.Builder and span.Splitter without table and
index descriptors.

Release note: None

rowexec: move eqCols to hashjoiner

This commit moves eqCols from joinerBase to hashJoiner because that is
the only processor that uses them. This simplifies the initialization
code for other processors.

Release note: None

sql: use IndexFetchSpec in JoinReader

This commit changes JoinReader to use an IndexFetchSpec (plus a
list of family IDs for span splitting) instead of table and index
descriptors.

As part of making this switch, the "internal schema" of the processor
no longer contains all table columns; instead it contains the specific
FetchedColumns in the spec. This simplifies both planning and
execution code.

Release note: None

@RaduBerinde RaduBerinde requested review from yuzefovich and a team February 24, 2022 01:04
@RaduBerinde RaduBerinde requested a review from a team as a code owner February 24, 2022 01:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Nice! I'm very happy to see the code in cfetcher_setup.go gone. However, there appears to be a regression in that we're using Scans instead of Gets now.

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 5 of 5 files at r3, 20 of 20 files at r4, 23 of 23 files at r5, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)


pkg/sql/distsql_physical_planner.go, line 2178 at r4 (raw file):

	}

	// Calculate the output columns from n.cols.

nit: the comment seems no longer applicable.


pkg/sql/colexec/colexecspan/span_assembler.go, line 31 at r4 (raw file):

// NewColSpanAssembler returns a ColSpanAssembler operator that is able to
// generate lookup spans from input batches.
// - neededColOrdsInWholeTable is a set containing the ordinals of all columns

nit: this should be now removed.


pkg/sql/opt/exec/execbuilder/testdata/autocommit_nonmetamorphic, line 646 at r5 (raw file):

----
dist sender send  r44: sending batch 2 CPut to (n1,s1):1
dist sender send  r44: sending batch 2 Scan to (n1,s1):1

This change is suspicious, any idea what this is about? There are similar changes in other files (like regional_by_row_query_behavior) where we seem to now have spans with EndKey prompting us to use Scan requests, instead of Gets.


pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic, line 372 at r5 (raw file):

  AND message NOT LIKE '%QueryTxn%'
----
dist sender send  r45: sending batch 42 Get to (n1,s1):1

This is not good.


pkg/sql/colexec/colexecspan/span_assembler_test.go, line 115 at r4 (raw file):

							colBuilder := NewColSpanAssembler(
								keys.TODOSQLCodec, testAllocator,

nit: the only line with two arguments.


pkg/sql/rowexec/joinerbase_test.go, line 85 at r4 (raw file):

			descpb.InnerJoin,
			tc.onExpr,
			nil,   /* leftEqColumns */

nit: this change should be in the third commit.

@RaduBerinde RaduBerinde force-pushed the join-reader-fetch-spec branch 2 times, most recently from 2d33067 to 1bf03b4 Compare February 24, 2022 04:09
Allow creation of span.Builder and span.Splitter without table and
index descriptors.

Release note: None
This commit moves eqCols from joinerBase to hashJoiner because that is
the only processor that uses them. This simplifies the initialization
code for other processors.

Release note: None
@RaduBerinde RaduBerinde force-pushed the join-reader-fetch-spec branch from 1bf03b4 to 6ed7184 Compare February 24, 2022 04:11
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Very nice catch, thank you for your careful review!! I forgot to initialize SplitFamilyIDs during planning.

I combed through the test changes now and they are only diagrams, plus a few cases where traces show more decoded columns (these cases are expected if we have an equality key column that is then projected away).

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)


pkg/sql/opt/exec/execbuilder/testdata/autocommit_nonmetamorphic, line 646 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This change is suspicious, any idea what this is about? There are similar changes in other files (like regional_by_row_query_behavior) where we seem to now have spans with EndKey prompting us to use Scan requests, instead of Gets.

Fixed.


pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic, line 372 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This is not good.

Fixed.

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.

I'm assuming you'll squash the fifth commit into the fourth, right?

:lgtm:

Reviewed 48 of 48 files at r6, 2 of 2 files at r7, 6 of 6 files at r8, 19 of 19 files at r9, 23 of 23 files at r10, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

This commit changes `JoinReader` to use an `IndexFetchSpec` (plus a
list of family IDs for span splitting) instead of table and index
descriptors.

As part of making this switch, the "internal schema" of the processor
no longer contains all table columns; instead it contains the specific
`FetchedColumns` in the spec. This simplifies both planning and
execution code.

Release note: None
@RaduBerinde RaduBerinde force-pushed the join-reader-fetch-spec branch from 6ed7184 to 170d49c Compare February 24, 2022 11:45
@RaduBerinde
Copy link
Member Author

Sure, done.

@RaduBerinde
Copy link
Member Author

bors r+

1 similar comment
@yuzefovich
Copy link
Member

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 25, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 25, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 25, 2022

Build succeeded:

@craig craig bot merged commit 94e64ce into cockroachdb:master Feb 25, 2022
@RaduBerinde RaduBerinde deleted the join-reader-fetch-spec branch March 19, 2022 01:33
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