sql: change encoding strategy for distsql datum hashing #45229

rohany · 2020-02-19T21:52:10Z

Row based distsql execution used the table key encoding
for datums when encoding them for operations like
distinct and aggregations. This is correct, but does
not work for types like JSONB or Arrays because these
types do not yet have valid table encodings. This PR
introduces a new Hash method on EncodedDatum that
uses the key encoding for indexable types, and defaults
to the value encoding for types that do not have
valid table keys. As a side effect, this PR
Fixes #44079, and allows for JSONB columns to be
aggregated and distincted.

Fixes #45216.

Release note (bug fix): Fix a bug where the distinct operation
on ARRAY[NULL] and NULL could sometimes
return an incorrect result and omit some tuples.

Release note (sql change): Allow for JSONB columns to be grouped
by and distincted on.

cockroach-teamcity · 2020-02-19T21:52:19Z

This change is

jordanlewis · 2020-02-19T21:53:19Z

Cool! This would also close #35706 and I think one or two other issues...

jordanlewis · 2020-02-19T21:57:22Z

Er, nevermind on the order by one. That wouldn't work. But I thought there was a group by json issue somewhere.

rohany · 2020-02-19T21:59:33Z

Matt had a PR out for ordering by json -- maybe we can pick it up.

jordanlewis · 2020-02-19T22:00:49Z

He did? Doesn't that mean he invented an order preserving JSON encoding which would mean we could also have ordinary JSON indexes?

rohany · 2020-02-19T22:01:43Z

take a look -- #43832

rohany · 2020-02-19T22:09:01Z

Fixes #45216

jordanlewis · 2020-02-19T22:11:02Z

You have to add the Fixes to the actual PR message.

Matt's PR I don't think helps here, because SQL order by also needs to use key encodings for stuff.

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @RaduBerinde, and @rohany)

pkg/sql/execinfra/server_config.go, line 63 at r1 (raw file):

// what changed.
const Version execinfrapb.DistSQLVersion = 26

Why do we have to change version here? Doesn't the hashing behave the same for all non-new data types?

rohany · 2020-03-02T16:03:00Z

No, we supported distinct/groupby on arrays before in distsql and now the hashing is different, so I think we want to bump the version

jordanlewis · 2020-03-02T17:05:27Z

Ah yeah you're right. My bad.

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rohany)

pkg/sql/logictest/testdata/logic_test/union, line 322 at r1 (raw file):

CREATE TABLE t2 (j JSONB);
INSERT INTO t1 VALUES ('{"a": "b"}');
INSERT INTO t2 VALUES ('{"c": "d"}');

do we want to union arrays too

pkg/sql/rowexec/version_history.txt, line 105 at r1 (raw file):

      IndexSkipTableReader, and InterleavedReaderJoiner spec.
- Version: 26 (MinAcceptedVersion: 26)
    - Change how datums are hashed for distinct and aggregation operations.

can you be specific that it's just arrays that changed

pkg/sql/sqlbase/encoded_datum.go, line 286 at r1 (raw file):

	case types.JsonFamily, types.ArrayFamily:
		// We must use value encodings without a column ID even if the EncDatum already
		// is encoded with the value encoding so that the hashes are indeed unique.

I don't think I understand this part - how come you can't just reuse the value encoding if it exists? aka using ed.Encode.

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @RaduBerinde)

pkg/sql/logictest/testdata/logic_test/union, line 322 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

do we want to union arrays too

Done.

pkg/sql/rowexec/version_history.txt, line 105 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

can you be specific that it's just arrays that changed

Done.

pkg/sql/sqlbase/encoded_datum.go, line 286 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I don't think I understand this part - how come you can't just reuse the value encoding if it exists? aka using ed.Encode.

Because table values are encoded with column offsets -- a different column offset on two datums would cause them to hash differently even if the datums were the same.

jordanlewis

once conflicts are resolved. @RaduBerinde would you mind taking a quick look as well? It's important we don't miss something here.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @RaduBerinde, and @rohany)

pkg/sql/sqlbase/encoded_datum.go, line 286 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Because table values are encoded with column offsets -- a different column offset on two datums would cause them to hash differently even if the datums were the same.

Gotcha.

RaduBerinde

How does this work for arrays of composite types? Does the following still give the same result?

root@127.171.54.131:38541/movr> create table d (d decimal);
CREATE TABLE

Time: 4.341782ms

root@127.171.54.131:38541/movr> insert into d values (0.0)i
root@127.171.54.131:38541/movr> insert into d values (1.0), (1.00), (1.000);
root@127.171.54.131:38541/movr> select array[d] from d;
   array
-----------
  {1.0}
  {1.00}
  {1.000}
(3 rows)

Time: 471.675µs

root@127.171.54.131:38541/movr> select distinct array[d] from d;
  array
---------
  {1.0}
(1 row)

BTW I first tried this with collated strings but hit #45703.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @RaduBerinde, and @rohany)

rohany · 2020-03-04T15:00:47Z

Oh wow, that doesn't work. Let me think about how to fix this.

rohany · 2020-03-04T16:05:11Z

Alright, I fixed that bug -- PTAL.

RaduBerinde

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

pkg/sql/logictest/testdata/logic_test/distinct_local, line 3 at r2 (raw file):

# LogicTest: local local-vec

# This test is moved from the distinct logictest file because when processed

I would keep it in that file and just wrap the query in a SELECT COUNT(*). And/or you could add another column and do SELECT DISTINCT ON (d) ORDER BY d,other

rohany · 2020-03-04T16:56:18Z

Done!

RaduBerinde

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

pkg/sql/sqlbase/encoded_datum.go, line 289 at r2 (raw file):

		return EncodeTableValue(appendTo, ColumnID(encoding.NoColumnID), ed.Datum, a.scratch)
	case types.ArrayFamily:
		// Arrays may contain composite data, so we cannot just value

value value

pkg/sql/sqlbase/encoded_datum.go, line 290 at r2 (raw file):

	case types.ArrayFamily:
		// Arrays may contain composite data, so we cannot just value
		// value encode an array, as that would give same-valued composite

[nit] put the third clause in a paren (that would ..)

pkg/sql/sqlbase/encoded_datum.go, line 276 at r3 (raw file):

}

// Hash appends a unique hash of ed to the given slice. If datums are intended

I don't think Hash is a good name, it normally implies a "lossy" function. Maybe Fingerprint?

pkg/sql/sqlbase/encoded_datum.go, line 279 at r3 (raw file):

// to be deduplicated or grouped with hashes, this function should be used
// instead of encode.
func (ed *EncDatum) Hash(typ *types.T, a *DatumAlloc, appendTo []byte) ([]byte, error) {

We are assuming another property of this function - that if we append a bunch of hashes, the result will still uniquely identify the set of values. This isn't the case in general though it is true of both key and value encodings. It should be called out here though.

For example, by the current definition it would be perfectly ok if Hash just printed out an integer as a string, but that won't work for arrays [1, 0] and [10].

Row based distsql execution used the table key encoding for datums when encoding them for operations like distinct and aggregations. This is correct, but does not work for types like JSONB or Arrays because these types do not yet have valid table encodings. This PR introduces a new `Hash` method on EncodedDatum that uses the key encoding for indexable types, and defaults to the value encoding for types that do not have valid table keys. As a side effect, this PR Fixes cockroachdb#44079, and allows for JSONB columns to be aggregated and distincted. Release note (bug fix): Fix a bug where the distinct operation on ARRAY[NULL] and NULL could sometimes return an incorrect result and omit some tuples. Release note (sql change): Allow for JSONB columns to be grouped by and distincted on.

rohany

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

pkg/sql/sqlbase/encoded_datum.go, line 286 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Gotcha.

Done.

pkg/sql/sqlbase/encoded_datum.go, line 276 at r3 (raw file):

Previously, RaduBerinde wrote…

I don't think Hash is a good name, it normally implies a "lossy" function. Maybe Fingerprint?

agreed, fingerprint sounds good.

rohany

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

pkg/sql/sqlbase/encoded_datum.go, line 290 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] put the third clause in a paren (that would ..)

Done

RaduBerinde

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

rohany · 2020-03-04T22:09:22Z

TFTR

bors r+

craig · 2020-03-04T22:28:41Z

Build failed (retrying...)

GitHub CI (Cockroach)

rohany · 2020-03-04T22:34:29Z

bors r-

craig · 2020-03-04T22:34:31Z

Canceled

rohany · 2020-03-04T22:34:42Z

I think a flake

bors r+

rohany · 2020-03-05T02:03:35Z

bors r+

craig · 2020-03-05T03:06:12Z

Build succeeded

GitHub CI (Cockroach)

46711: rowexec: fix bug with distinct aggregations on JSONB columns r=yuzefovich a=rohany Fixes #46709. A recent change (#45229) was made to fix encodings to perform aggregations on JSONB and ARRAY columns. That change missed a case in for DISTINCT aggregations, causing a runtime error when executing these queries. Release justification: fixes a bug Release note (bug fix): This PR fixes a bug with distinct aggregations on JSONB columns. Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>

49851: rowcontainer: fix hash row container for some types r=yuzefovich a=yuzefovich The explanation is that `HashDiskRowContainer` is implemented using `DiskRowContainer` with the equality columns (i.e. the columns to hash) of the former being the ordering columns for the latter, and those ordering columns are used to compute the keys of the rows (in `encodeRow`) so that we could store the row in the sorted order. This way we store the build (right) side of the join, but for the probe (left) side we use `hashMemRowIterator` to compute the key of the probing row. The key computation methods must be the same in both places, otherwise, the results of the join can be incorrect. #45229 broke this synchronization by changing the key computation method in `hashMemRowIterator.computeKey` to use `Fingerprint`. So we have to either use `Fingerprint` in `encodeRow` or use `Encode` in `computeKey`. The first choice doesn't seem to work because `Fingerprint` doesn't provide the ordering we need in `DiskRowContainer`, so we need to use the second approach. The ordering property is necessary because `DiskRowContainer` implements "hash row container" by sorting all rows on the ordering (i.e. hash) columns and using the ordering property to provide the "hashing" behavior (i.e. we would seek to the first row that has the same hash columns and then iterate from that row one row at a time forward until the hash columns remain the same). If we don't have the ordering property, then the necessary invariant that all rows that hash to the same value are contiguous is not maintained. Release note: None Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

rohany requested review from jordanlewis and RaduBerinde February 19, 2020 21:52

rohany force-pushed the dist-agg branch from 1d89a12 to 408ff3c Compare February 19, 2020 22:00

jordanlewis reviewed Mar 2, 2020

View reviewed changes

rohany force-pushed the dist-agg branch from 408ff3c to f2cdb1c Compare March 2, 2020 18:58

rohany commented Mar 2, 2020

View reviewed changes

jordanlewis approved these changes Mar 4, 2020

View reviewed changes

RaduBerinde reviewed Mar 4, 2020

View reviewed changes

rohany force-pushed the dist-agg branch from f2cdb1c to ccc3bf8 Compare March 4, 2020 16:04

RaduBerinde reviewed Mar 4, 2020

View reviewed changes

rohany force-pushed the dist-agg branch from ccc3bf8 to f10fcca Compare March 4, 2020 16:56

RaduBerinde reviewed Mar 4, 2020

View reviewed changes

rohany commented Mar 4, 2020

View reviewed changes

rohany force-pushed the dist-agg branch from f10fcca to b08d8f4 Compare March 4, 2020 17:54

rohany commented Mar 4, 2020

View reviewed changes

RaduBerinde approved these changes Mar 4, 2020

View reviewed changes

craig bot merged commit f3e1b90 into cockroachdb:master Mar 5, 2020

jseldess mentioned this pull request Mar 19, 2020

sql: change encoding strategy for distsql datum hashing cockroachdb/docs#6915

Closed

rohany mentioned this pull request Mar 28, 2020

rowexec: fix bug with distinct aggregations on JSONB columns #46711

Merged

yuzefovich mentioned this pull request Jun 3, 2020

sql: enable simple stats for all types #49801

Merged

yuzefovich mentioned this pull request Jun 3, 2020

rowcontainer: fix hash row container for some types #49851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: change encoding strategy for distsql datum hashing #45229

sql: change encoding strategy for distsql datum hashing #45229

rohany commented Feb 19, 2020 •

edited

Loading

cockroach-teamcity commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

rohany commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

rohany commented Feb 19, 2020

rohany commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

jordanlewis left a comment

rohany commented Mar 2, 2020

jordanlewis commented Mar 2, 2020

jordanlewis left a comment

rohany left a comment

jordanlewis left a comment

RaduBerinde left a comment

rohany commented Mar 4, 2020

rohany commented Mar 4, 2020

RaduBerinde left a comment

rohany commented Mar 4, 2020

RaduBerinde left a comment

rohany left a comment

rohany left a comment

RaduBerinde left a comment

rohany commented Mar 4, 2020

craig bot commented Mar 4, 2020

rohany commented Mar 4, 2020

craig bot commented Mar 4, 2020

rohany commented Mar 4, 2020

rohany commented Mar 5, 2020

craig bot commented Mar 5, 2020

sql: change encoding strategy for distsql datum hashing #45229

sql: change encoding strategy for distsql datum hashing #45229

Conversation

rohany commented Feb 19, 2020 • edited Loading

cockroach-teamcity commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

rohany commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

rohany commented Feb 19, 2020

rohany commented Feb 19, 2020

jordanlewis commented Feb 19, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

rohany commented Mar 2, 2020

jordanlewis commented Mar 2, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany commented Mar 4, 2020

rohany commented Mar 4, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany commented Mar 4, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rohany commented Mar 4, 2020

craig bot commented Mar 4, 2020

Build failed (retrying...)

rohany commented Mar 4, 2020

craig bot commented Mar 4, 2020

Canceled

rohany commented Mar 4, 2020

rohany commented Mar 5, 2020

craig bot commented Mar 5, 2020

Build succeeded

rohany commented Feb 19, 2020 •

edited

Loading