Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for casting tdigest to text and vice versa #1

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

thanodnl
Copy link
Collaborator

As discussed; we ran into an issue when you cast a tdigest to text and later back to tdigest.

The char *centroids would only contain the data till the first word break. Instead we introduce the use of %n to count the length of the data captured by the header. This gets added to the char *str and captured in ptr before parsing the centroids on their own.

While working on this patch I also ran into pq_getmsgint(buf, sizeof(int64)); being called. According to the documentation in pqformat.c Postgres decided to not implement the 64 bit variant:

/* --------------------------------
 *		pq_getmsgint64	- get a binary 8-byte int from a message buffer
 *
 * It is tempting to merge this with pq_getmsgint, but we'd have to make the
 * result int64 for all data widths --- that could be a big performance
 * hit on machines where int64 isn't efficient.
 * --------------------------------
 */

I think this gets used when you store a tdigest in a relation, so I also added a test at the end that will store some tdigest data in a table that gets combined into the final tdigest before verifying the correctness of the results.

@tvondra tvondra merged commit e2a866b into tvondra:master Jun 10, 2020
@tvondra
Copy link
Owner

tvondra commented Jun 10, 2020

Thanks. I've squashed the commits and merged it like that.

Nice catch with the pg_getmsgint call. I wonder if we might test that somehow (the existing tests only check the text format, I think), but the only way I can think of is COPY with (FORMAT binary), which does not seem particularly nice in regression tests.

@min-mwei min-mwei mentioned this pull request Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants