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

Relax bounds on aggregate functions? #1272

Open
marcusllorente-msft opened this issue Aug 24, 2023 · 6 comments
Open

Relax bounds on aggregate functions? #1272

marcusllorente-msft opened this issue Aug 24, 2023 · 6 comments
Labels
blocked enhancement New feature or request question Further information is requested

Comments

@marcusllorente-msft
Copy link

marcusllorente-msft commented Aug 24, 2023

Hi - new to Rust and PGRX, so excuse me if my questions are a bit silly. I am trying to create an aggregate function in PGRX and I am working with the aggregate example code to get a better understanding. My goal right now is to read a column that contains text in PGSQL and to insert each piece of text into a vector that is a field in the aggregate state. However, when trying to initialize the field in the state struct, it gives me issues with some of the traits and types. This is the code I am working with now - it is the same as the aggregate example but I am just adding a vector field inside the struct to continue with testing.

use core::ffi::CStr;
use pgrx::aggregate::*;
use pgrx::prelude::*;
use pgrx::{pgrx, PgVarlena, PgVarlenaInOutFuncs, StringInfo};
use serde::{Deserialize, Serialize};
use std::str::FromStr;

pgrx::pg_module_magic!();

#[derive(Copy, Clone, PostgresType, Serialize, Deserialize)]
#[pgvarlena_inoutfuncs]
pub struct IntegerAvgState {
    sum: i32,
    n: i32,
    inputs: Vec<String>,
}
(The rest of the code is copied and pasted from the aggregates lib.rs code)

When attempting to run, I get this error:
19 | #[derive(Copy, Clone, PostgresType, Serialize, Deserialize)] | ^^^^ ... 24 | inputs: Vec<String>, | ------------------- this field does not implement std::marker::Copy``

In reference to the inputs field I added in the struct.
Any feedback would be super appreciated. Thank you so much!

@eeeebbbbrrrr
Copy link
Contributor

Vev<String> doesn't derive Copy, so your IntegerAvgState struct can't either. That's just a Rust thing.

I don't believe pgrx' aggregate support requires the state function to derive Copy, so you should be able to remove it.

@eeeebbbbrrrr eeeebbbbrrrr added the question Further information is requested label Aug 24, 2023
@marcusllorente-msft
Copy link
Author

marcusllorente-msft commented Aug 24, 2023

When I tried to remove it, it errors saying that the trait bound IntegerAvgState: std::marker::Copy is not satisfied with the following details:

97  | pub struct PgVarlena<T>
    |            --------- required by a bound in this struct
98  | where
99  |     T: Copy + Sized,
    |        ^^^^ required by this bound in `PgVarlena`

@eeeebbbbrrrr
Copy link
Contributor

Okay, upon further investigation, our aggregate state function is bound by Copy. My bad. I can't tell you why off the top of my head tho.

Basically, that means your "state" struct needs to be a fixed size and not contain anything that also does heap allocation, such as a Vec (or a String or a HashMap, etc ,etc).

It's not clear to me what you're trying to accomplish by storing inputs: Vec<String>, but perhaps you can find another approach?

@marcusllorente-msft
Copy link
Author

Ahhh, the fixed size constraint makes sense. I'm gonna see if I can try to find a work around. Thank you for your help!

@workingjubilee workingjubilee changed the title Working with aggregates and creating a vector Relax bounds on aggregate functions Aug 25, 2023
@workingjubilee workingjubilee changed the title Relax bounds on aggregate functions Relax bounds on aggregate functions? Aug 25, 2023
@workingjubilee workingjubilee added the enhancement New feature or request label Aug 25, 2023
@workingjubilee
Copy link
Member

workingjubilee commented Aug 25, 2023

After I finish with the List rework I will take a look at aggregates. Unless someone beats me to it.

@workingjubilee
Copy link
Member

Investigating this is waiting for the new MemCx API to land, because the relaxation won't be sound otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants