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

internal/keyspan: add Value to Span #1382

Merged
merged 1 commit into from
Nov 23, 2021
Merged

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Nov 23, 2021

Add a Value to the Span type, copying it to both fragments at a
fragmentation point. This will be used in the implementation of range
keys, which will be fragmented but carry a value.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Just nits. :lgtm:

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jbowens and @sumeerbhola)


internal/keyspan/fragmenter_test.go, line 20 at r1 (raw file):

`(\d+):\s*(\w+)-*(\w+) *([^\n]*)`

nit: can the whitespace character after the third capture group be an \s? Slightly more explicit. i.e.:

(\d+):\s*(\w+)-*(\w+)\s*([^\n]*)

internal/keyspan/testdata/fragmenter_values, line 71 at r1 (raw file):

2: a---e                      banana
1: a-----g                    coconut
flush-to d

I had to squint to spot the differences between this test case and the one prior (flush vs. truncate-and-flush). Might be worth a brief comment above one or both of the test cases to call out the difference explicitly.

Also, unrelated to the test case itself, and more to the docs on FlushTo, I was a little confused as to why the c-e spans were flushed. Reading inside the function itself, it seems that we "flush all span fragments with a start key <= key". Can we repeat that comment on the function signature itself?


internal/keyspan/testdata/fragmenter_values, line 85 at r1 (raw file):

1:     e-g                    coconut
3:       g-i                  orange

supernit: extra trailing newline

Add a Value to the `Span` type, copying it to both fragments at a
fragmentation point. This will be used in the implementation of range
keys, which will be fragmented but carry a value.
Copy link
Collaborator Author

@jbowens jbowens 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: 13 of 16 files reviewed, 2 unresolved discussions (waiting on @nicktrav and @sumeerbhola)


internal/keyspan/fragmenter_test.go, line 20 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…
`(\d+):\s*(\w+)-*(\w+) *([^\n]*)`

nit: can the whitespace character after the third capture group be an \s? Slightly more explicit. i.e.:

(\d+):\s*(\w+)-*(\w+)\s*([^\n]*)

Nice, done.


internal/keyspan/testdata/fragmenter_values, line 71 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

I had to squint to spot the differences between this test case and the one prior (flush vs. truncate-and-flush). Might be worth a brief comment above one or both of the test cases to call out the difference explicitly.

Also, unrelated to the test case itself, and more to the docs on FlushTo, I was a little confused as to why the c-e spans were flushed. Reading inside the function itself, it seems that we "flush all span fragments with a start key <= key". Can we repeat that comment on the function signature itself?

Good call, done.

@jbowens jbowens merged commit 613e08e into cockroachdb:master Nov 23, 2021
@jbowens jbowens deleted the value-span branch November 23, 2021 22:18
Copy link
Collaborator

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

Reviewed 12 of 16 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions


internal/keyspan/testdata/fragmenter, line 25 at r2 (raw file):

1:          j--m
2:             m-----s
1:             m-----s------z

why did this change?

Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions


internal/keyspan/testdata/fragmenter, line 25 at r2 (raw file):

Previously, sumeerbhola wrote…

why did this change?

The deleted portion ------z was ignored by the parsing regex and nonsensical here, where each line indicates 1 new fragment.

@jbowens jbowens mentioned this pull request Nov 30, 2021
29 tasks
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