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

feat: support array #130

Merged
merged 1 commit into from
Dec 4, 2023
Merged

feat: support array #130

merged 1 commit into from
Dec 4, 2023

Conversation

xhwhis
Copy link
Contributor

@xhwhis xhwhis commented Dec 1, 2023

Array Support

This PR introduces support Array type, specifically validated with DataFusion. While I'm uncertain whether this qualifies as a formal PR, it has proven beneficial for my use case. I modify the datafusion.rs example to facilitate the validation of this PR.

Additionally, I removed the ToSqlText impl for u8, u16, and u64. In my view, impl ToSqlText should align with the corresponding ToSql types in the postgres-types crate, and providing ToSqlText for these three types could be misleading. I would appreciate your thoughts on this matter.

If there are unfinished tasks to be addressed or if you have any suggestions, please let me know. I am willing to collaborate to complete the support for Array type.

@sunng87
Copy link
Owner

sunng87 commented Dec 2, 2023

Thank you so much for this patch!

Additionally, I removed the ToSqlText impl for u8, u16, and u64. In my view, impl ToSqlText should align with the corresponding ToSql types in the postgres-types crate, and providing ToSqlText for these three types could be misleading. I would appreciate your thoughts on this matter.

The reason behind this is there is no unsigned integer in postgresql. (u32 was treated as objectId). But when using pgwire as a gateway to build your own "postgresql implementation", it is possible to have unsigned integer support. At least I don't think there is significant drawback with these types supported in ToSqlText.

@@ -64,7 +64,7 @@ impl SimpleQueryHandler for DfSessionService {
Ok(vec![Response::Error(Box::new(ErrorInfo::new(
"ERROR".to_owned(),
"XX000".to_owned(),
"Datafusion is a readonly execution engine. To load data, call\nLOAD csv_file_path table_name;".to_owned(),
"Datafusion is a readonly execution engine. To load data, call\nLOAD json_file_path table_name;".to_owned(),
Copy link
Owner

Choose a reason for hiding this comment

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

By the way, could you please include a JSON sample file as reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like this

{"a":[1,2,3],"b":[2.0,1.3,-6.1],"c":[false, true]}
{"a":[4,5,7],"b":[1.6,6.9,2.34],"c":[true, true]}
{"a":[2,5,2],"b":[0.1,1.9,-2.3],"c":[false, false]}

@xhwhis
Copy link
Contributor Author

xhwhis commented Dec 2, 2023

Thank you so much for this patch!

Additionally, I removed the ToSqlText impl for u8, u16, and u64. In my view, impl ToSqlText should align with the corresponding ToSql types in the postgres-types crate, and providing ToSqlText for these three types could be misleading. I would appreciate your thoughts on this matter.

The reason behind this is there is no unsigned integer in postgresql. (u32 was treated as objectId). But when using pgwire as a gateway to build your own "postgresql implementation", it is possible to have unsigned integer support. At least I don't think there is significant drawback with these types supported in ToSqlText.

I think we should add comment doc to guide users to convert when they need unsigned types. When we use encode_field we cannot directly encode u8,u16,u64 because they do not impl ToSql. As long as we use DataRowEncoder to process data, we cannot handle u8,u16,u64. It makes little sense to users. The more important point is that if you impl ToSqlText for u8, then when you impl ToSqlText for &[T], it will conflict with &[u8](bytea). So at least you need to remove the ToSqlText impl of u8. Otherwise I can only use macro to each array type impl ToSqlText, which is unreasonable in my opinion.

@sunng87
Copy link
Owner

sunng87 commented Dec 4, 2023

The more important point is that if you impl ToSqlText for u8, then when you impl ToSqlText for &[T], it will conflict with &u8.

I see. That's fair reason for u8 implementation removal.

By the way, I sent an email to you for your use-case of pgwire. Remember to check. Thank you.

@sunng87 sunng87 merged commit 8d91b27 into sunng87:master Dec 4, 2023
6 checks passed
@xhwhis xhwhis deleted the support-array branch December 4, 2023 08:47
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.

2 participants