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

Outbound pg host component #622

Merged
merged 7 commits into from
Jul 28, 2022
Merged

Outbound pg host component #622

merged 7 commits into from
Jul 28, 2022

Conversation

miketang84
Copy link
Contributor

@miketang84 miketang84 commented Jul 9, 2022

This is a Postgresql host component used for the interaction with an outside Pg database. This component is implemented according to the paradigm of outbound-redis.

Welcome to review and help me improve this work, thank you.

In the example test, the schema of the pg table is:

postgres=# \d articletest 
                   Table "public.articletest"
   Column   |       Type        | Collation | Nullable | Default 
------------+-------------------+-----------+----------+---------
 title      | character varying |           |          | 
 content    | character varying |           |          | 
 authorname | character varying |           |          | 

@fibonacci1729
Copy link
Contributor

Thank you! This is awesome! I will take it for a spin on monday!

@fibonacci1729
Copy link
Contributor

Just got around to this, everything works great! I'll walk through the code a little more to see if i can offer any more feedback, but so far 💯 really nice work!

Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes but otherwise LGTM!

examples/rust-outbound-pg/src/lib.rs Outdated Show resolved Hide resolved
examples/rust-outbound-pg/src/lib.rs Outdated Show resolved Hide resolved
@fibonacci1729
Copy link
Contributor

One last item, make sure to cargo fmt and cargo clippy just to ensure you catch any lint's before CI. Thanks!

@fibonacci1729
Copy link
Contributor

@miketang84 one last thing, before merging you'll need to sign your commits. But otherwise, i think this is ready to merge (once CI passes)!

@miketang84 miketang84 force-pushed the outbound-pg branch 4 times, most recently from 6afe50e to 8fdf978 Compare July 15, 2022 02:18
@miketang84
Copy link
Contributor Author

@miketang84 one last thing, before merging you'll need to sign your commits. But otherwise, i think this is ready to merge (once CI passes)!

Rebase and sign commits. Done it.

Signed-off-by: Mike Tang <daogangtang@gmail.com>
Signed-off-by: Mike Tang <daogangtang@gmail.com>
Signed-off-by: Mike Tang <daogangtang@gmail.com>
Signed-off-by: Mike Tang <daogangtang@gmail.com>
Signed-off-by: Mike Tang <daogangtang@gmail.com>
let sql = "select * from articletest";
let rows = pg::query(&address,sql, &vec![]).map_err(|_| anyhow!("Error execute pg command"))?;

println!("rows: {:?}", rows);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we give an example here (or start some draft documentation) of how to use payloads? I don't have an environment to run this but reading the code it looks like the payloads are raw bytes, and it's not obvious to me how to translate those into the component language (Rust, Go, Swift, etc.).

(I'm also a bit unsure about identifying columns in a select * query like this, but that would be easy to work around in a production sample by listing the columns explicitly.)

Despite this nit I am very excited to see this! Thank you for putting it together!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let me look into it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we give an example here (or start some draft documentation) of how to use payloads? I don't have an environment to run this but reading the code it looks like the payloads are raw bytes, and it's not obvious to me how to translate those into the component language (Rust, Go, Swift, etc.).

(I'm also a bit unsure about identifying columns in a select * query like this, but that would be easy to work around in a production sample by listing the columns explicitly.)

Despite this nit I am very excited to see this! Thank you for putting it together!

Hi, I updated the example code and add some type converts for the result vec payload. Please help me to check on it.

Appreciated for your kind suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super helpful - thanks! However I think consumers are still going to need some guidance on how to convert other types e.g. numerics, booleans, SQL NULLs, etc. (I'm not familiar with the full range of Postgres types). They don't need to be all in the sample (that would be a bit much!) but they need to be in the documentation somewhere. (If you want, feel free to just jot things down, and we can turn them into public documentation.)

Do you think it's worth baking those conversion helpers into the SDKs? I guess we can always add them over time!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your comments.

The full support against to the postgres types is not the task of this PR, in my thought, later I will look into the crate postgres-types, suppose it will cover the abstraction and convertions between the lower bytes payload and the upper rust types, I wish we could use it in an automatically converting style, just like rust-postgres does.

That's a lot of work, but if you want that, I think I can do it later based on the current work of this PR.

Regards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, I think it's better to implement the convertor layer in the rust SDK rather than in the host component layer. Cause that

  • the complexity of the *.wit file;
  • the updated frequency of the host component;
  • the potential other language SDK must recognize fully the output of the host component.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply. I'm okay with a "merge and iterate" strategy - happy to get this in and get things motoring. Thanks!

For that this is the first concept verified version, we didn't utilize the
library like postgres-types and serde.

Signed-off-by: Mike Tang <daogangtang@gmail.com>
Signed-off-by: Mike Tang <daogangtang@gmail.com>
@itowlson itowlson merged commit 360ad62 into fermyon:main Jul 28, 2022
@itowlson
Copy link
Contributor

itowlson commented Aug 4, 2022

Kia ora @miketang84, any thoughts on documenting how to interpret the payload blobs (other than strings)? I'm wanting to do some SDK work to surface this. I'm happy to put together formal documentation but if you could provide some starter info that would be super useful. Thanks!

@miketang84
Copy link
Contributor Author

Hi, @itowlson, let me try it.

@miketang84 miketang84 deleted the outbound-pg branch August 6, 2022 08:46
@itowlson
Copy link
Contributor

itowlson commented Aug 8, 2022

@miketang84 I ran into a limitation while experimenting with a new SDK, so have proposed a revision to the interface in #677. Would love your feedback. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants