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

row: fetcher cleanup and improvements #75261

Merged
merged 4 commits into from
Jan 22, 2022

Conversation

RaduBerinde
Copy link
Member

rowenc: remove deprecated return from DecodeIndexKey

Release note: None

row: minor cleanup around foundNull

This moves around some code to make it clear that it's only relevant in
a specific case.

Release note: None

row: clean up ReadIndexKey

Renaming to DecodeIndexKey and removing return value which is no
longer useful.

Release note: None

row: more Fetcher cleanup

  • improve comment for indexKey;
  • unexport NextKey;
  • slightly change the return value of nextKey to simplify the logic
    (the semantic difference is what the first call returns, which is
    not used);
  • use numKeysPerRow instead of counting the total families; this
    enables the faster paths for more cases.

Release note: None

@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.

:lgtm:

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


pkg/sql/catalog/descpb/BUILD.bazel, line 14 at r1 (raw file):

        "descriptor.go",
        "index.go",
        "index_fetch.pb.go",

Changes in this file seem suspicious.


pkg/sql/row/fetcher.go, line 731 at r3 (raw file):

}

// DecodeIndexKey decodes an index key and returns the remaining key and wheter

nit: s/wheter/whether/.

This moves around some code to make it clear that it's only relevant
in a specific case.

Release note: None
Renaming to DecodeIndexKey and removing return value which is no
longer useful.

Release note: None
 - improve comment for `indexKey`;
 - unexport NextKey;
 - slightly change the return value of nextKey to simplify the logic
   (the semantic difference is what the first call returns, which is
   not used);
 - use numKeysPerRow instead of counting the total families; this
   enables the faster paths for more cases.

Release note: None
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.

TFTR!

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


pkg/sql/catalog/descpb/BUILD.bazel, line 14 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Changes in this file seem suspicious.

Nice catch, it was a leftover pb.go file from another branch

@RaduBerinde
Copy link
Member Author

Test failure is an unrelated flake.

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 21, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Jan 21, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jan 21, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jan 22, 2022

Build succeeded:

@craig craig bot merged commit fdfd893 into cockroachdb:master Jan 22, 2022
@RaduBerinde RaduBerinde deleted the row-fetcher-cleanup branch January 27, 2022 20:51
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