Update tdigest_aggregate_support output for PG15+ #7849
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-13.0 #7849 +/- ##
================================================
- Coverage 89.48% 89.42% -0.06%
================================================
Files 276 276
Lines 60063 60062 -1
Branches 7524 7523 -1
================================================
- Hits 53747 53711 -36
- Misses 4166 4198 +32
- Partials 2150 2153 +3 |
naisila
approved these changes
Jan 17, 2025
|
Quick question: would this work with PG14 as well? (given that we haven't dropped support for it) |
colm-mchugh
force-pushed
the
cmchugh/release-13.0-fix-tdigest-regress
branch
from
f0a3b0d to
6f8423a
Compare
Citus 11.0 on PG14 was failing with different query results, possibly because of minor differences in floating-point calculations. Added an alternative regress output file for this, for completeness. |
colm-mchugh
force-pushed
the
cmchugh/release-13.0-fix-tdigest-regress
branch
from
6f8423a to
7181bcc
Compare
Regress test tdigest_aggregate_support has been failing since at least Citus 11.0, because of an ommission by citus commit 03832f3 and a change in the implementation of Postgres random() function (pg commit d4f109e4a) To reproduce the test diff: - Checkout tdigest from https://github.com/tvondra/tdigest and `make; make install` - In citus regress directory run `make check-multi` or `./citus_tests/run_test.py tdigest_aggregate_support` There are two parts to this commit: 1) Revert "Output: xxxxx" in EXPLAIN VERBOSE output. Citus commit fe4ac51 normalized EXPLAIN VERBOSE output because of a change between pg12 and pg13. When pg12 support was no longer required, the rule was removed from normalize.sed and "Output: xxxx" was reverted in the impacted regress output files (03832f3), but tdigest_aggregate_support was ommitted (by 03832f3). 2) Adjust query results; the tdigest_aggregate_support test file has a comment: '-- verifying results - should be stable due to seed while inserting the data, if failure due to data these queries could be removed or check for certain ranges', but the result values in this commit are consistent across citus 12.0 (pg 15), citus 12.1 (pg 16) and citus 13.0 (pg 17), or since the Postgres commit that changed the implementation of random: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d4f109e4a so proposing to go with these results. Note: citus 11.0 (PG14.12) was failing with slightly different query results, possibly due to floating point difference; added an alternative regress output file for PG14.
colm-mchugh
force-pushed
the
cmchugh/release-13.0-fix-tdigest-regress
branch
from
7181bcc to
5425e0e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Regress test tdigest_aggregate_support has been failing since at least Citus 12.0, when tdigest extension is installed in Postgres. This appears to be because of an omission by commit 03832f3 and a change in the implementation of Postgres random() function (pg commit d4f109e4a). To reproduce the test diff:
make; make installmake check-multior./citus_tests/run_test.py tdigest_aggregate_supportThere are two parts to this commit:
Revert
Output: xxxxxin EXPLAIN VERBOSE. Citus commit fe4ac51 normalized EXPLAIN VERBOSE output because of a change between pg12 and pg13. When pg12 support was no longer required, the rule was removed from normalize.sed andOutput: xxxxwas reverted in the impacted regress output files (03832f3), buttdigest_aggregate_supportwas omitted.Adjust the query results; the tdigest_aggregate_support test file has a comment verifying results - should be stable due to seed while inserting the data, if failure due to data these queries could be removed or check for certain ranges but the result values in this commit are consistent across citus 12.0 (pg 15), citus 12.1 (pg 16) and citus 13.0 (pg 17), or since the Postgres changed their implementation of random, so proposing to go with these results.