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 bugs with non default hll fields #7

Merged
merged 1 commit into from
Sep 8, 2020

Conversation

markallanson
Copy link

The implementation of hll fields with non default values was broken
as it passed the hll kwargs down to the Field without removing them.

Note there is a wider batch of work to support non default values in
the library as new hll fields created, such as HllEmpty need to match
the settings of the field being written to. I have not attempted to
address this, and have simply used the hll_set_defaults to control
things in the tests.

The implementation of hll fields with non default values was broken
as it passed the hll kwargs down to the Field without removing them.

Note there is a wider batch of work to support non default values in
the library as new hll fields created, such as HllEmpty need to match
the settings of the field being written to. I have not attempted to
address this, and have simply used the hll_set_defaults to control
things in the tests.
@M1ha-Shvn
Copy link
Owner

Hi.
Thanks for your PR. Where can I find specification of hll_empty parameters you write about? I can't even see hll_set_defaults function in official postgres-hll extension README. May be I have missed something?

@M1ha-Shvn M1ha-Shvn added bug Something isn't working enhancement New feature or request and removed enhancement New feature or request labels Sep 8, 2020
@M1ha-Shvn
Copy link
Owner

I've found a reference here. I'll accept your PR and release version 1.2.1 as a bug fix and create separate issue for creating extra functionality where you can watch and comment progress or may be collaborate in future, if you are interested in it.

@M1ha-Shvn M1ha-Shvn added this to the 1.2.1 milestone Sep 8, 2020
@M1ha-Shvn M1ha-Shvn merged commit 8e59f05 into M1ha-Shvn:master Sep 8, 2020
@M1ha-Shvn
Copy link
Owner

I'll make a release of 1.2.1 a little bit later, as pypi returns "500 Internal Server error" for now when I try uploading packages...

@markallanson
Copy link
Author

Thanks. Yeah a lot of this is not documented. You can see the hll_empty stuff in hll.c#2508 where there are variations of the function that allow you to define the params.

Essentially the types you create have to match up to the configuration of the target hll column. It's going to be quite a tricky beast to implement it thoroughly in the client library.

@markallanson markallanson deleted the fix/custom_params branch September 8, 2020 16:23
@markallanson
Copy link
Author

I'll make a release of 1.2.1 a little bit later, as pypi returns "500 Internal Server error" for now when I try uploading packages...

Any chance you could try pushing this to pipy again?

@M1ha-Shvn
Copy link
Owner

I've tried in the morning, still getting error. I've written an issue to twine repository as I found same closed issues there. Now I wait for their answer. Similar issues found special characters in sources or README.md. But I can't even upload to testpypi previous versions, which have been uploaded successfully to production pypi a long time ago. Strange.

@M1ha-Shvn
Copy link
Owner

Released v1.2.1

@markallanson
Copy link
Author

markallanson commented Sep 13, 2020

Thanks a lot - really appreciate the effort to push this through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants