tool: Use the comparator-specified Formatter if there is one

[itsbilal]

If the key formatter falls back to calling printf on the key,
and there's an in-use pebble.Comparator with a specified Format
function, then use that format function instead.

[petermattis]

This change is 

[itsbilal]

This is a regression - think I'll have to special case the %s case in fmtFormatter or something.

[petermattis]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @petermattis)

---

tool/testdata/sstable_scan, line 48 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

This is a regression - think I'll have to special case the %s case in fmtFormatter or something.

I think this rules should be something like:

• If you don't specify --key=, or you specify --key=pretty use cmp.Format if it is non-nil, otherwise use FormatBytes.
• If you specify --key=x, use the formatter x

You can know if --key= is specified on the command line by adding a formatter.isSet boolean which is set to true whenever formatter.Set is called. We use this same technique for the --log-dir flag in CockroachDB.

[itsbilal]

@petermattis Good idea. I've implemented that rule as-is, PTAL!

[petermattis]

The output looks good. I have some comments about the implementation. It is possible I'm missing something about the need for the current structure.

I believe this doesn't leave a way to get pretty-printed keys from wal dump. Perhaps we should allow --key=pretty[:<comparer-name>] to explicitly request the formatter for a specific comparer.

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @itsbilal)

---

tool/sstable.go, line 171 at r2 (raw file):

			// Update the internal formatter if this comparator has one specified.
			for i := range s.comparers {

Rather than doing this unconditionally, I was imagining you'd condition this on s.fmtKey.setByUser. Something like:

if !s.fmtKey.setByUser {
  // Find formatter matching the comparer name...
}

---

tool/util.go, line 76 at r2 (raw file):

		f.fn = formatQuoted
	case "pretty":
		f.fn = func(v []byte) fmt.Formatter {

I was imagining this would be f.fn = base.FormatBytes. Then when you the comparer is known, if it has a Format field you set f.fn = comparer.Format. The logic to leave f.setByUser = false would remain as that allows higher level code to know that comparer.Format should be set if one is available.

---

tool/util.go, line 104 at r2 (raw file):

}

type fmtFormatter struct {

It feels odd to me to be using fmtFormatter for this purpose. fmtFormatter is intended to call fmt.Fprintf. I think you should add a new prettyFormatter.

[itsbilal]

Added the pretty:<comparer-name> part as well, which (if specified and valid), takes precedence over the comparator formatter. PTAL!

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @itsbilal and @petermattis)

---

tool/sstable.go, line 171 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Rather than doing this unconditionally, I was imagining you'd condition this on s.fmtKey.setByUser. Something like:

if !s.fmtKey.setByUser {
  // Find formatter matching the comparer name...
}

Done.

---

tool/util.go, line 76 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I was imagining this would be f.fn = base.FormatBytes. Then when you the comparer is known, if it has a Format field you set f.fn = comparer.Format. The logic to leave f.setByUser = false would remain as that allows higher level code to know that comparer.Format should be set if one is available.

Done.

---

tool/util.go, line 104 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

It feels odd to me to be using fmtFormatter for this purpose. fmtFormatter is intended to call fmt.Fprintf. I think you should add a new prettyFormatter.

Done. The previous change for directly setting formatter.fn in the right cases lets us ditch a dedicated struct for this altogether. No need for prettyFormatter.

[petermattis]

I think there is a small bit of simplification in the implementation possible. Otherwise this looks good to go.

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @itsbilal)

---

tool/manifest.go, line 102 at r3 (raw file):

				}
				if len(m.fmtKey.comparer) > 0 {
					for i := range m.comparers {

m.comparers is a map keyed by comparer name. I think you can do something like:

if c := m.comparers[m.fmtKey.comparer]; c != nil && c.Format != nil {
  m.fmtKey.fn = c.Format
}

---

tool/sstable.go, line 312 at r3 (raw file):

			// Update the internal formatter if this comparator has one specified.
			if !s.fmtKey.setByUser || len(s.fmtKey.comparer) > 0 {

Looks like this code is duplicated somewhere else. I think you could pull it out into a utility function in utils.go.

---

tool/testdata/wal_dump, line 63 at r3 (raw file):

wal dump
../testdata/db-stage-4/000006.log
--key=pretty:test-comparer

Neat!

[itsbilal]

Was able to simplify a lot of the repeated with a setter function in util.go. Thanks for the review!

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @itsbilal)

[petermattis]

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @itsbilal)

---

tool/util.go, line 124 at r4 (raw file):

	}

	if cmp, ok := comparers[comparerName]; ok && cmp != nil && cmp.Format != nil {

It is fine to use , ok here, but also unnecessary. A map will return the zero value for a lookup when a key isn't present. So comparers["nonExistentComparer"] will return nil.

[itsbilal]

Thanks for the review!

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @itsbilal and @petermattis)

---

tool/util.go, line 124 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

It is fine to use , ok here, but also unnecessary. A map will return the zero value for a lookup when a key isn't present. So comparers["nonExistentComparer"] will return nil.

Sounds good, fixed! For some reason I thought maps panicked if they were accessed with only one return value.