Nice work so far @C0rWin, thanks!

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

pkg/sql/logictest/testdata/logic_test/aggregate, line 2466 at r1 (raw file):

----
{"Alice": {"Facebook": "Alice_fb", "Instagram": "AliceInst", "Twitter": "@Alice"}}
{"Bob": {"Facebook": "Bob_fb", "LinkedIn": "Bob The Builder"}}

Can you add some tests with the error cases you wrote in here? Checks for nulls, as an example. Also, please include some examples with JSON nulls, JSON arrays, and JSON numbers.

pkg/util/json/json.go, line 246 at r1 (raw file):

}

// ObjectBuilderWithCounter builds JSON object which keeps track of total size

nit: this should be a complete sentence with a period at the end. How about:

ObjectBuilderWithCounter builds a JSON object a key/value pair at a time, keeping of the memory usage of the object.

pkg/util/json/json.go, line 252 at r1 (raw file):

}

// NewObjectBuilderWithCounter creates and instantiate ObjectBuilder with memory counter

nit: add a period

pkg/util/json/json.go, line 266 at r1 (raw file):

func (b *ObjectBuilderWithCounter) Add(k string, v JSON) {
	b.ob.Add(k, v)
	// Size of added JSON + size of the map kay pair and length of the key

nit: "kay" should be key.

pkg/util/json/json_test.go, line 467 at r1 (raw file):

		{
			input:    [][]interface{}{{"key1", []interface{}{1, 2, 3, 4}}},
			expected: json(`{"key1": [1, 2, 3, 4]}`),

Please add some test cases with nulls.

Couple more comments, but this is nearly there, thank you Artem!

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

pkg/sql/logictest/testdata/logic_test/aggregate, line 2470 at r2 (raw file):

#empty result returned, since both key and value is nil, hence skipped
statement ok
select json_object_agg(null, null);

I think even this should return an error actually... any time the first arg is null you should return the error below.

pkg/sql/logictest/testdata/logic_test/aggregate, line 2528 at r2 (raw file):

{"Alice": {"Facebook": "Developer", "Google": "Full Stack", "IBM": "Research"}}
{"Bob": {"Cockroach Labs": "DB Developer", "Google": "DevOps", "Twitter": "Frontend"}}
{"Charlie": null}

Are there any tests for what happens when there's a duplicate object key? Please add tests for that, thanks!

pkg/sql/sem/builtins/aggregate_builtins.go, line 2685 at r2 (raw file):

	// if only key datum equal to null, return error
	if datum == tree.DNull {

Bring this up above the first check, it should error if we ever have a null in the first position.

Thanks for working on this! I left a bunch of nits, but in general the code is almost ready to go.

I have some reservation with respect to duplicate keys, as I do not think this is defined or strictly articulated. While I understand that this has to be aligned with Postgres, it still not very well defined ...

Here is an excerpt from Postgres docs:

The aggregate functions array_agg, json_agg, jsonb_agg, json_object_agg, jsonb_object_agg,
string_agg, and xmlagg, as well as similar user-defined aggregate functions, produce meaningfully
different result values depending on the order of the input values. This ordering is unspecified by
default, but can be controlled by writing an ORDER BY clause within the aggregate call.

So when there is no ORDER BY clause in the aggregate function, we are free to choose any order. If we deviate from Postgres in such case, it's ok, meaning that we should not be concerned about this:

the way CRDB currently treats JSON object formation, i.e. order of keys and order among duplicate keys depends on insertion order, hence not really deteministic for aggregate functions.

In fact, in the logic tests you added we already deviate from Postgres, but it's acceptable.

On a related note, we should also add json_object_agg and jsonb_object_agg to isOrderingSensetive in optbuilder/groupby.go. This will allow us to support these new aggregates with ORDER BY clause (using window functions machinery behind the scenes), and we should add the tests for that.

Reviewed 1 of 11 files at r1, 2 of 7 files at r2, 8 of 8 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @C0rWin)

pkg/sql/logictest/testdata/logic_test/aggregate, line 2638 at r3 (raw file):

statement error pq: field name must not be null
select json_object_agg(null, null);

super nit: unnecessary semicolon.

pkg/sql/opt/operator.go, line 306 at r3 (raw file):

		ConstNotNullAggOp, CorrOp, FirstAggOp, JsonAggOp, JsonbAggOp,
		MaxOp, MinOp, SqrDiffOp, StdDevOp, StringAggOp, SumOp, SumIntOp,
		VarianceOp, XorAggOp, PercentileDiscOp, PercentileContOp, JsonObjectAggOp, JsonbObjectAggOp:

super nit: seems like it might be time to start a new line here and below.

pkg/sql/sem/builtins/aggregate_builtins.go, line 3051 at r3 (raw file):

}

// Add accumulates the transformed json into the JSON array.

nit: probably s/JSON array/JSON object/.

pkg/sql/sem/builtins/aggregate_builtins.go, line 3059 at r3 (raw file):

	}

	// if only key datum equal to null, return error

nit: if we have a comment that lives on a separate line, we usually start it with a capital letter and end it with a period.

pkg/util/json/json.go, line 246 at r3 (raw file):

}

// ObjectBuilderWithCounter builds a JSON object a key/value pair at a time, keeping of the memory usage of the object.

nit: s/keeping of the/keeping the/ and we strive for lines no longer than 80 characters.

pkg/util/json/json.go, line 252 at r3 (raw file):

}

// NewObjectBuilderWithCounter creates and instantiate ObjectBuilder with memory counter.

nit: s/instantiate/instantiates/.

pkg/util/json/json.go, line 263 at r3 (raw file):

// Add appends key value pair to the sequence and updates
// amount of memory allocated for the overall keys and values

nit: missing period.

pkg/util/json/json.go, line 266 at r3 (raw file):

func (b *ObjectBuilderWithCounter) Add(k string, v JSON) {
	b.ob.Add(k, v)
	// Size of added JSON + size of the map key pair and length of the key

nit: something is off with "map key pair and length of the key", maybe "Size of added JSON + overhead of storing key/value pair + the size of the key"? (Also, a missing period.)

pkg/util/json/json.go, line 276 at r3 (raw file):

}

// Size returns the size in bytes of the JSON Array the builder is going to build.

nit: probably s/Array/object/.

The code looks good to me, but seems like you accidentally deleted a bunch of logic test cases, we should keep those :)

Reviewed 8 of 8 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @C0rWin)

pkg/sql/logictest/testdata/logic_test/aggregate, line 2570 at r3 (raw file):

query TT
SELECT min(NULL::STRING COLLATE en_us), max(NULL::STRING COLLATE en_us)

Seems like you accidentally deleted a bunch of test cases here.

Reviewed 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @C0rWin and @jordanlewis)

pkg/sql/logictest/testdata/logic_test/aggregate, line 2528 at r2 (raw file):

Hm, currently we deviate from Postgres in how we handle duplicate keys. Postgres seems to keep all of them whereas we keep only the last one.

PG:

yuzefovich=# CREATE TABLE profiles (
  userid int not null,
  property text not null,
  value text not null
);
CREATE TABLE
yuzefovich=# INSERT INTO profiles VALUES (1, 'email', 'foo'), (1, 'email', 'bar');
INSERT 0 2
yuzefovich=# SELECT json_object_agg(property, value) FROM profiles GROUP BY userid ORDER BY userid;
           json_object_agg            
--------------------------------------
 { "email" : "foo", "email" : "bar" }
(1 row)

CRDB:

root@127.0.0.1:50547/defaultdb> SELECT json_object_agg(property, value) FROM profiles GROUP BY userid ORDER BY userid;
  json_object_agg
--------------------
  {"email": "bar"}
(1 row)

I believe it is an existing behavior of jsonObjectBuilder, and this PR simply adds another case for when we use that object. @jordanlewis I think it should be ok to merge this, what do you think?

Ok, cool,

I left last couple of nits, and once those are addressed, I'll merge this.

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

pkg/sql/opt/optbuilder/groupby.go, line 828 at r5 (raw file):

		return b.factory.ConstructPercentileCont(args[0], args[1])
	case "json_object_agg":
		return b.factory.ConstructJsonbObjectAgg(args[0], args[1])

nit: the calls are mismatched with the respective cases.

pkg/sql/sem/builtins/aggregate_builtins.go, line 3059 at r5 (raw file):

	}

	// If only key datum equal to null, return error.

nit: there is something slightly off with the comment, I'd probably replace it with something like "If the key datum is NULL, return an error."

pkg/sql/sem/builtins/aggregate_builtins.go, line 3081 at r5 (raw file):

}

// Result returns an DJSON from the array of JSON.

nit: s/an DJSON/a DJSON/.

pkg/util/json/json.go, line 246 at r3 (raw file):

Ping.

Thanks for your contribution!

bors r=yuzefovich

Reviewed 3 of 3 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @C0rWin and @jordanlewis)