Vectorized hash grouping by a single text column

[akuzm]

Use the UMASH hashes that have a guaranteed lower bound on collisions as the hash table keys.

Up to 70% improvement in tsbench: https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=4078&var-run2=4080&var-threshold=0.02&var-use_historical_thresholds=true&var-threshold_expression=2%20%2A%20percentile_cont%280.90%29&var-exact_suite_version=false&from=now-2d&to=now

[codecov]

Codecov Report

Attention: Patch coverage is 90.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.44%. Comparing base (59f50f2) to head (6252dfe).
Report is 760 commits behind head on main.

Files with missing lines	Patch %	Lines
...des/vector_agg/hashing/hash_strategy_single_text.c	88.88%	1 Missing and 4 partials ⚠️
tsl/src/nodes/vector_agg/plan.c	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7586      +/-   ##
==========================================
+ Coverage   80.06%   81.44%   +1.38%     
==========================================
  Files         190      245      +55     
  Lines       37181    44976    +7795     
  Branches     9450    11217    +1767     
==========================================
+ Hits        29770    36632    +6862     
- Misses       2997     3954     +957     
+ Partials     4414     4390      -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[erimatnor]

LGTM. I added some suggestions for minor improvements. Also have some general questions.

[erimatnor]

Instead of defining a new type, can we use StringInfo and initReadOnlyStringInfo? Just an idea.

[akuzm]

Well, it's not a complex type, and StringInfo has some unrelated things, so I'd keep it as is.

[erimatnor]

Question for my own understanding (not asking for any changes for this PR):

This is deep into vector aggregation, so I would expect that this function only gets passed an arrow array. But now we need to check for different non-array formats, including impossible cases (e.g, DT_Iterator). If we only passed in arrays, these checks would not be necessary.

The arrow array format already supports everything we need. Even scalar/segmentby values can be represented by arrow arrays (e.g., run-end encoded).

Now we need this extra code to check for different formats/cases everywhere we reach into the data. Some of them shouldn't even be possible here.

IMO, the API to retrieve a value should be something:

Datum d = arrow_array_get_value_at(array, rownum, &isnull, &valuelen);

This function can easily check the encoding of the array (dict, runend, etc.) to retrieve the value requested.

[akuzm]

Yeah, I have already regretted supporting the scalar values throughout aggregation. Technically it should perform better, because it avoids creating e.g. arrow array with the same constant value for every row, and sometimes we perform the computations in a different way for scalar values. But the implementation complexity might be a little too high. Maybe I should look into removing this at least for the key column, and always materializing them into arrow arrays. I'm going to consider this after we merge the multi-column aggregation.

The external interface might still turn out to be more complex than what you suggest, and closer to the current CompressedColumnValues, because sometimes we have to statically generate the function that works e.g. with dictionary encoding specifically, and that won't be possible if we determine this inside an opaque callback. We can't call an opaque callback (i.e. a non-inlinable dynamic function) for every row because it's going to produce significantly less performant code.

[akuzm]

This function can easily check the encoding of the array (dict, runend, etc.) to retrieve the value requested.

This is not enough though, the arrow arrays don't know their value size/type, for example. They should always be accompanied by some metadata.

[erimatnor]

Suggest making use of PostgreSQL builtin function:

Suggested change

	const int total_bytes = output_key.len + VARHDRSZ;
	text *restrict stored = (text *) MemoryContextAlloc(policy->hashing.key_body_mctx, total_bytes);
	SET_VARSIZE(stored, total_bytes);
	memcpy(VARDATA(stored), output_key.data, output_key.len);
	MemoryConext oldmcxt = MemoryContextSwitchTo(policy->hashing.key_body_mctx);
	text *stored = cstring_to_text_with_len(output_key.data, output_key.len);
	MemoryContextSwitchTo(oldmcxt);

[akuzm]

I think it's better to have something that can be inlined here, because it's a part of a hot loop that builds the hash table.

[erimatnor]

Why do we need to skip these tests on windows? Is it also because of UMASH?

[akuzm]

Right, I didn't get it to compile there, so decided to disable for now.