sql: add backward compatibility to stats and plan

[maryliag]

Previously, if a key didn't exist on a JSON payload for
statement/transaction stats and plan, the decoder would return
an error and the page would not load on Console.
This commit changes the behaviour, returning the value as
nil, so the page can continue to load normally and only that
particular value would be kept as empty.

Fix #70599

Release Justification: Category 2, bug fix
Release note: None

[cockroach-teamcity]

This change is 

[Azhng]

Let's add some tests in pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go.

I'm thinking we can use the similar tests here.

More concretely, I'm think we can probably use the same tests input, and use fillTemplate() (for the test input) and fillObject() (for the expected output) to populate them with random data. Then manually remove fields from the resulting JSON object using .RemovePath(), then zeros out the corresponding fields in our expected output, then re-decode the JSON to see if the output matches our expectation.

tempalte := "..."
expectedObj := roachpb.CollectedTransactionStatistics{}
data := genRandomData()

inputStr := fillTemplate(t, expectedStatisticsStrTemplate, data)
fillObject(t, reflect.ValueOf(&expectedObj), &data)

inputJSON, err := json.ParseJSON(inputStr)
require.NoError(t, err)

inputJSON.RemovePath(/* get rid of some fields */)
expectedObj.Stats.RowsWritten = roachpb.NumericStats{} // zeros out some fields

// now check if inputJSON after being decoded matches the expectedObj

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_decoding.go, line 95 at r1 (raw file):

					return nil, err
				}
				if childJSON != nil {

How do you feel about invert the if condition here to be more consistent with the golang styles?

if childJSON == nil {
  // ... sad path
}

// ... happy path

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_impl.go, line 303 at r1 (raw file):

			return err
		}
		if field != nil {

nit: same here

[maryliag]

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

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_decoding.go, line 95 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

How do you feel about invert the if condition here to be more consistent with the golang styles?

if childJSON == nil {
  // ... sad path
}

// ... happy path

if the value is nil we don't do anything, so adding that if nil would be an empty condition inside, so no point on adding the check

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_impl.go, line 303 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: same here

same as above, that's make sense the change

[maryliag]

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

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_impl.go, line 303 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

same as above, that's make sense the change

*that doesn't make sense to change

[maryliag]

Test added :)

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

[Azhng]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_decoding.go, line 91 at r2 (raw file):

		if key == "Children" {
			for childIdx := 0; childIdx < value.Len(); childIdx++ {
				childJSON, err := value.FetchValIdx(childIdx)

hmm, do we need to remove safeFetchValIdx here? Since we are looping using childIdx < value.Len() here, each value of the JSON array should not be nil, (otherwise the length is incorrect, which can be a bug).

I'm slightly leaning towards to keeping the safeFetchValIdx but still removing safeFetchVal

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 154 at r2 (raw file):

	t.Run("statement_statistics with new parameter", func(t *testing.T) {
		data := genRandomData()
		input := roachpb.CollectedStatementStatistics{}

nit: i think we should call this expected (or something like that) since we are not using it as input for decoding

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 175 at r2 (raw file):

         "maxRetries":      {{.Int64}},
         "lastExecAt":      "{{stringifyTime .Time}}",
				 "numRows": {

nit: hmm, the indentation seems weird here, perhaps issue with tab vs space ?

[maryliag]

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

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_decoding.go, line 91 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

hmm, do we need to remove safeFetchValIdx here? Since we are looping using childIdx < value.Len() here, each value of the JSON array should not be nil, (otherwise the length is incorrect, which can be a bug).

I'm slightly leaning towards to keeping the safeFetchValIdx but still removing safeFetchVal

as you mentioned, it will always find the value since it's checking it's own indexes, so the safeFetch isn't doing anything, meaning it is safe to use the fetch directly and only add if the value was indeed not null. No need for an extra function that won't add anything (would be the equivalent to adding a check for every single for we have)

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 154 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: i think we should call this expected (or something like that) since we are not using it as input for decoding

Done

---

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 175 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: hmm, the indentation seems weird here, perhaps issue with tab vs space ?

weird... fixed

[Azhng]

🔥

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng)

[maryliag]

bors r+

[craig]

Build failed:

• GitHub CI (Cockroach)

[maryliag]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)