-
-
Notifications
You must be signed in to change notification settings - Fork 253
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
feat: add aggregate example #187
Conversation
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Based on some feedback from @eeeebbbbrrrr I migrated this to using |
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 really great Ana. I have a couple of questions and a suggestion or two, for my own understanding mostly, but nothing that would block merging. Well done.
.DS_Store | ||
.idea/ | ||
/target | ||
*.iml |
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.
out of interest, what's an .iml
file?
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 dunno!
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.
It’s a CLion project file
@@ -0,0 +1,5 @@ | |||
comment = 'aggregate: Created by pgx' |
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.
Thought: perhaps we should have a URL that people can visit so they know what pgx is i they encounter it in their logs?
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 suggest opening an issue if you feel passionate about this in a general case! I think most users would set this to what their Cargo.toml
description is!
pgx-examples/aggregate/src/lib.rs
Outdated
pg_module_magic!(); | ||
|
||
|
||
use serde::{Deserialize, Serialize}; |
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.
Is this where we want this import?
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 keep forgetting we don't run rustfmt
on CI...
|
||
let mut split = input.to_bytes().split(|b| *b == b','); | ||
let sum = split.next().map(|value| | ||
i32::from_str(unsafe { std::str::from_utf8_unchecked(value) }).expect("invalid i32") |
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.
In the error message, should we match the Postgres type name?
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'm not sure this is important, it's an fairly small example. :)
pgx-examples/aggregate/src/lib.rs
Outdated
buffer.push_str(&self.sum.to_string()); | ||
buffer.push(','); | ||
buffer.push_str(&self.n.to_string()); |
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.
Personal preference only. Slightly easier to skim read.
buffer.push_str(&self.sum.to_string()); | |
buffer.push(','); | |
buffer.push_str(&self.n.to_string()); | |
buffer.push_str(&format!("{},{}"), self.sum, self.n); |
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.
That would be doing allocations in what might be a hot path for Postgres. The idea of StringBuffer is to save that.
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.
One of the reasons I considered using format was that it would only involve 1 allocation rather than 2 (each call to .to_string()
).
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.
Fair. Push it
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Adds a aggregate example as demonstrated by @laysakura in #102 . Thanks!!!