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

proposal: support casting between PgNodes? #1046

Open
skyzh opened this issue Feb 16, 2023 · 3 comments
Open

proposal: support casting between PgNodes? #1046

skyzh opened this issue Feb 16, 2023 · 3 comments
Labels
enhancement New feature or request ffi-bindgen

Comments

@skyzh
Copy link
Contributor

skyzh commented Feb 16, 2023

As I'm working on a foreign data wrapper, I'll need to traverse the plan tree, and de-parse it to extract the information inside it. Currently, I'm manually implementing casting for the types I use:

pub trait PgNodeExt: PgNode {
    unsafe fn as_restrict_info(&self) -> &pg_sys::RestrictInfo;
    unsafe fn as_op_expr(&self) -> &pg_sys::OpExpr;
    unsafe fn as_var(&self) -> &pg_sys::Var;
    unsafe fn as_const(&self) -> &pg_sys::Const;
    unsafe fn as_relabel_type(&self) -> &pg_sys::RelabelType;
    unsafe fn as_bool_expr(&self) -> &pg_sys::BoolExpr;
}


impl<T: PgNode> PgNodeExt for T {
    unsafe fn as_restrict_info(&self) -> &pg_sys::RestrictInfo {
        if is_type(self, pg_sys::NodeTag_T_RestrictInfo) {
            unsafe { mem::transmute::<_, &pg_sys::RestrictInfo>(self) }
        } else {
            panic!("Not a RestrictInfo");
        }
    }

    unsafe fn as_op_expr(&self) -> &pg_sys::OpExpr {
        if is_type(self, pg_sys::NodeTag_T_OpExpr) {
            unsafe { mem::transmute::<_, &pg_sys::OpExpr>(self) }
        } else {
            panic!("Not a opexpr");
        }
    }

    unsafe fn as_bool_expr(&self) -> &pg_sys::BoolExpr {
        if is_type(self, pg_sys::NodeTag_T_BoolExpr) {
            unsafe { mem::transmute::<_, &pg_sys::BoolExpr>(self) }
        } else {
            panic!("Not a boolexpr");
        }
    }

    unsafe fn as_var(&self) -> &pg_sys::Var {
        if is_type(self, pg_sys::NodeTag_T_Var) {
            unsafe { mem::transmute::<_, &pg_sys::Var>(self) }
        } else {
            panic!("Not a var");
        }
    }

    unsafe fn as_const(&self) -> &pg_sys::Const {
        if is_type(self, pg_sys::NodeTag_T_Const) {
            unsafe { mem::transmute::<_, &pg_sys::Const>(self) }
        } else {
            panic!("Not a const");
        }
    }

    unsafe fn as_relabel_type(&self) -> &pg_sys::RelabelType {
        if is_type(self, pg_sys::NodeTag_T_RelabelType) {
            unsafe { mem::transmute::<_, &pg_sys::RelabelType>(self) }
        } else {
            panic!("Not a relabeltype");
        }
    }
}

I'm wondering if it would be good to build this inside pg_sys? In the build.rs stage we can generate something like:

pub trait PgNodeExt: PgNode {
    unsafe fn as_<something>(&self) -> &pg_sys::Something; // for all PgNode types, panic if type mismatch
    unsafe fn as_<something>_mut(&mut self) -> &mut pg_sys::Something;
    unsafe fn try_as_<something>(&self) -> Option<&pg_sys::Something>; // return None if type mismatch
    unsafe fn try_as_<something>_mut(&mut self) -> Option<&mut pg_sys::Something>;
}

Or we can implement it as From and TryFrom.

impl <T: PgNode> From<&T> for <SomeNode> {} // repeat for all pgnodes
impl <T: PgNode> TryFrom<&T> for <SomeNode> {} // repeat for all pgnodes

I can have a try if this sounds like a good idea.

@eeeebbbbrrrr
Copy link
Contributor

I like the From/TryFrom idea quite a bit.

Do you think it can be 100% machine generated? I've got just enough experience walking the plan tree to think that it could be. My main concern with this is that we don't want to have to maintain handwritten code for this, especially across 5 Postgres versions.

So if you think it can be 100% generated, yeah, give it a shot. We'll most likely accept the patch.

Also note that we've been working on a complete retooling of how we generate bindings. It's not clear to me what impact that'll have to post-processing work such as this, but it'll still be doable -- it may just be that we'll need to rework your patch.

@eeeebbbbrrrr eeeebbbbrrrr added ffi-bindgen enhancement New feature or request labels Feb 16, 2023
@skyzh
Copy link
Contributor Author

skyzh commented Feb 16, 2023

I'll investigate if we can make them 100% machine generated and have a try. Will update in this thread.

@skyzh
Copy link
Contributor Author

skyzh commented Feb 16, 2023

I have a simple demo here -> #1048, let see if it passes CI...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ffi-bindgen
Projects
None yet
Development

No branches or pull requests

2 participants