Fix variable resolution in vectorized aggregation planning #7415

akuzm · 2024-11-05T12:47:05Z

We didn't properly resolve INDEX_VARs in the output targetlist of DecompressChunk nodes, which are present when it uses a custom scan targetlist. Fix this by always working with the targetlist where these variables are resolved to uncompressed chunk variables, like we do during execution.

Fixes #7410 (probably, I couldn't reproduce the original issue).

We didn't properly resolve INDEX_VARs in the output targetlist of DecompressChunk nodes, which are present when it uses a custom scan targetlist. Fix this by always working with the targetlist where these variables are resolved to uncompressed chunk variables, like we do during execution.

github-actions · 2024-11-05T12:47:29Z

@gayyappan, @erimatnor: please review this pull request.

Powered by pull-review

codecov · 2024-11-05T22:31:41Z

Codecov Report

Attention: Patch coverage is 77.77778% with 10 lines in your changes missing coverage. Please review.

Project coverage is 82.09%. Comparing base (59f50f2) to head (106a68f).
Report is 601 commits behind head on main.

Files with missing lines	Patch %	Lines
tsl/src/nodes/vector_agg/plan.c	79.06%	2 Missing and 7 partials ⚠️
tsl/src/nodes/vector_agg/exec.c	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7415      +/-   ##
==========================================
+ Coverage   80.06%   82.09%   +2.02%     
==========================================
  Files         190      230      +40     
  Lines       37181    43112    +5931     
  Branches     9450    10838    +1388     
==========================================
+ Hits        29770    35391    +5621     
- Misses       2997     3396     +399     
+ Partials     4414     4325      -89

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

tsl/src/nodes/vector_agg/plan.c

erimatnor · 2024-11-11T11:07:53Z

tsl/test/expected/vector_agg_param.out

+                                 Output: compress_hyper_2_4_chunk._ts_meta_count, compress_hyper_2_4_chunk.s, compress_hyper_2_4_chunk._ts_meta_min_1, compress_hyper_2_4_chunk._ts_meta_max_1, compress_hyper_2_4_chunk.a
+(20 rows)
+
+select * from unnest(array[0, 1, 2]::int[]) x, lateral (select sum(a + x) from pvagg) xx;


Not sure what this is testing. Is it just checking that the query doesn't fail (if it did fail prior to this fix)? Or is it testing that it gives correct output?

How can I know that the output (sum) is correct? Is there a non-compressed (regular) table I can compare with?

This is testing an aggregate function reference that has an expression that references a nested loop parameter.

I generated a reference by running the same query with vectorized aggregation disabled.

Shouldn't the reference output be part of the test? Now the set for turning off vector agg is a commented line so I cannot see the reference or verify that this is correct.

You uncomment this line and this generates the reference in the output file -- all queries run with normal postgres aggregation and not vectorized aggregation. Then you comment it back and run the test again and check that nothing else changes. You do this once when changing the test, I already done this, the test output has the correct results generated by standard Postgres plan. This is the approach I use for some other tests as well.

The pattern we have for this is to generate two output files and do a diff between them in the test. There are examples in other tests how to do this.

Having this comparison of the outputs is good because it also easily captures future errors and regressions.

The pattern we have for this is to generate two output files and do a diff between them in the test. There are examples in other tests how to do this.

Yeah, I know, I don't like to use it because:

The test runs more than twice slower

Inconvenient to view the entire test reference, you have to do extra steps to open another file for this.

The PG version is not always the same as our version, e.g. some vectorized functions have better numeric stability.

This is the stuff that I remember off the top of my head, probably there are more reasons. Why is it a problem?

Oh, you also have to put the actual test queries into a separate file and run it with psql, so editing a test is also more complicated.

Having this comparison of the outputs is good because it also easily captures future errors and regressions.

The test I wrote also compares the outputs, only the PG output is fixed at the test editing time.

When you generate the output each time, you make it effectively compare the four different supported PG versions against each other. Not sure what's the benefit, probably you'll just run into some numeric stability change in PG and will have to painfully work around it.

The pattern we have for this is to generate two output files and do a diff between them in the test. There are examples in other tests how to do this.

Yeah, I know, I don't like to use it because:

1. The test runs more than twice slower 2. Inconvenient to view the entire test reference, you have to do extra steps to open another file for this. 3. The PG version is not always the same as our version, e.g. some vectorized functions have better numeric stability.

This is the stuff that I remember off the top of my head, probably there are more reasons. Why is it a problem?

I am merely giving feedback on things I think would improve the test and avoid regression, as well as for my own understanding so that I don't just approve without understanding what is going on. Only now, after I asked, it is clear that the test output is in fact different from regular PG aggregates, as you admit. Even if it is not strictly wrong, I cannot verify this in the review. It gives me pause because it was neither documented nor clear from the test. At the very least, this could have been good information to provide in the test. Having different aggregate output also means we can't easily capture regressions and it requires someone to know that they need to manually enable and inspect the output when something changes, which I think you are currently the only person who knows how to do easily.

Ideally, our tests should be easy to understand and maintain also by others, this is the perspective I have. Is there some way we can improve the test to make it easier for others to understand the aspects above?

Only now, after I asked, it is clear that the test output is in fact different from regular PG aggregates, as you admit.

That's not in this test, that's in different ones where I also use this pattern. What regressions do you want to avoid? This is the usual "golden test", it runs some queries and compares their output against the one captured in the reference. Most our tests are like that. Here we also have a possibility to compare the reference against the analogous PG output by uncommenting a single line in this test. What should be improved here?

erimatnor · 2024-11-11T11:12:51Z

tsl/src/nodes/vector_agg/plan.c

+						   ->expr);
+		Assert(var->varno > 0);
+
+		return (Node *) copyObject(var);


Why do we need a copyObject() here but not in the other return cases? Or, to ask it differently, should we docopyObject() also in the other return of var?

This has no practical consequences here, but it is idiomatic for the expression tree mutators to return a copy. I added copyObject into the second place as well.

Co-authored-by: Erik Nordström <[email protected]> Signed-off-by: Alexander Kuzmenkov <[email protected]>

erimatnor · 2024-11-14T13:40:52Z

tsl/test/expected/vector_agg_param.out

@@ -21,23 +21,38 @@ select count(compress_chunk(x)) from show_chunks('pvagg') x;
 (1 row)

 analyze pvagg;
-explain (costs off)
+-- Uncomment to generate reference
+--set timescaledb.enable_vectorized_aggregation to off;


Should this be here?

Yes, this is just for conveniently generating the reference using the standard Postgres plans whenever you want to change this test.

erimatnor · 2024-11-14T13:43:51Z

tsl/test/expected/vector_agg_param.out

+                                 Output: compress_hyper_2_4_chunk._ts_meta_count, compress_hyper_2_4_chunk.s, compress_hyper_2_4_chunk._ts_meta_min_1, compress_hyper_2_4_chunk._ts_meta_max_1, compress_hyper_2_4_chunk.a
+(20 rows)
+
+select * from unnest(array[0, 1, 2]::int[]) x, lateral (select sum(a + x) from pvagg) xx;


Shouldn't the reference output be part of the test? Now the set for turning off vector agg is a commented line so I cannot see the reference or verify that this is correct.

github-actions bot requested review from erimatnor and gayyappan November 5, 2024 12:47

akuzm added 4 commits November 5, 2024 13:49

changelog

bc30ab8

typo

69ed75d

fix

da0e3de

silence the warning

eecd1dd

test

edaa7cb

svenklemm approved these changes Nov 7, 2024

View reviewed changes

Merge branch 'main' into resolve

eddcddc

akuzm enabled auto-merge (squash) November 7, 2024 14:40

akuzm added 2 commits November 7, 2024 16:31

Merge remote-tracking branch 'origin/main' into HEAD

a5dfada

fix

d11fec5

erimatnor reviewed Nov 11, 2024

View reviewed changes

akuzm and others added 6 commits November 12, 2024 10:44

Update tsl/src/nodes/vector_agg/plan.c

92c6fd3

Co-authored-by: Erik Nordström <[email protected]> Signed-off-by: Alexander Kuzmenkov <[email protected]>

fixes

d5ad761

Merge remote-tracking branch 'origin/main' into HEAD

1992b8e

copy

c6eb880

fix

0c59d9a

fixes for pg16

e71a015

erimatnor reviewed Nov 14, 2024

View reviewed changes

akuzm added 3 commits November 18, 2024 13:22

Merge remote-tracking branch 'origin/main' into HEAD

31bceb9

remove accidental change

d4c97f0

update test ref

fa9c11f

akuzm mentioned this pull request Nov 19, 2024

Vectorized hash grouping on one column #7316

Draft

7 tasks

update the comment

106a68f

fabriziomello assigned akuzm Nov 20, 2024

antekresic approved these changes Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix variable resolution in vectorized aggregation planning #7415

Fix variable resolution in vectorized aggregation planning #7415

akuzm commented Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024

codecov bot commented Nov 5, 2024 •

edited

Loading

erimatnor Nov 11, 2024

akuzm Nov 13, 2024

erimatnor Nov 14, 2024

akuzm Nov 14, 2024

erimatnor Nov 18, 2024 •

edited

Loading

akuzm Nov 18, 2024

akuzm Nov 18, 2024

akuzm Nov 18, 2024

erimatnor Nov 19, 2024

akuzm Nov 19, 2024

erimatnor Nov 11, 2024

akuzm Nov 13, 2024

erimatnor Nov 14, 2024

akuzm Nov 14, 2024

erimatnor Nov 14, 2024

Fix variable resolution in vectorized aggregation planning #7415

Are you sure you want to change the base?

Fix variable resolution in vectorized aggregation planning #7415

Conversation

akuzm commented Nov 5, 2024 • edited Loading

github-actions bot commented Nov 5, 2024

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erimatnor Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akuzm commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

erimatnor Nov 18, 2024 •

edited

Loading