-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add count_min_sketch
aggregate and approx_count
accessor
#458
Conversation
#[pg_extern(immutable, parallel_safe, schema = "toolkit_experimental")] | ||
pub fn approx_count(item: String, aggregate: Option<CountMinSketch>) -> i64 { | ||
let sketch = aggregate.unwrap(); | ||
CountMinSketch::to_internal_countminsketch(&sketch).estimate(item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the antipattern that is my biggest grievance with how we use the crates subdirectory. Converting the Postgres CountMinSketch object back into the crate object so that we can call a function on it is really not efficient. We can leave this in place for now, but we'll need to implement the estimate functionality directly on top of the Postgres structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having implemented this two or three times so far myself, I couldn't agree more that it's a mess! But I think the answer is to break extension
up into more crates, so that we can build business logic directly against pg_type structs without all this destruction and reconstruction and so that we can avoid giving rustc an ever-growing pile of files to handle when I make a one-line change to only one of those files. What stops us now is so much of what goes into a pg_type
is locked inside the extension
crate.
We've nibbled at the edges of this discussion a few times... how about I spend a day to prototype something like this when I get back from vacation?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be interested to see that!
9d6ef7f
to
5e792c0
Compare
b6e3064
to
63490fc
Compare
5e792c0
to
22ca881
Compare
ccc69fb
to
10da4f3
Compare
22ca881
to
4c2fefe
Compare
10da4f3
to
276bc46
Compare
0bb64e3
to
43944c2
Compare
c328c10
to
f316426
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one last suggestion.
43944c2
to
564d6c4
Compare
f316426
to
cde5010
Compare
564d6c4
to
b93ec95
Compare
cde5010
to
4c4628e
Compare
bors r+ |
Build succeeded: |
Add
count_min_sketch
aggregate andapprox_count
accessor.The initial implementations work only on
text
columnsResolves #373