Query binders: Add support for postgres arrays #336
Comments
|
@nitnelave do you want to create PR for this issue? |
|
I don't actually have a need for this, I just wanted to file this for posterity and/or completeness sake. |
|
So what I am wondering is what is our status for keep drivers and binders? I think keeping both is an unnecessary burden. |
|
@Sytten maybe we can drop drivers from SeaQuery in the future? Now mark it as deprecated and add support other drivers into binder. |
|
I think I got an idea for the implementation: use crate::{SqlxValue, SqlxValues};
use sea_query::Value;
impl<'q> sqlx::Encode<'q, sqlx::postgres::Postgres> for SqlxValue {
fn encode_by_ref(&self, buf: &mut sqlx::postgres::PgArgumentBuffer) -> sqlx::encode::IsNull {
match &self.0 {
Value::Bool(b) => {
<Option<bool> as sqlx::Encode<'q, sqlx::postgres::Postgres>>::encode_by_ref(&b, buf)
}
Value::TinyInt(i) => {
<Option<i8> as sqlx::Encode<'q, sqlx::postgres::Postgres>>::encode_by_ref(&i, buf)
}
Value::SmallInt(i) => {
todo!()
}
Value::Int(i) => {
todo!()
}
Value::BigInt(i) => {
todo!()
}
Value::TinyUnsigned(i) => {
todo!()
}
Value::SmallUnsigned(i) => {
todo!()
}
Value::Unsigned(i) => {
todo!()
}
Value::BigUnsigned(i) => {
todo!()
}
Value::Float(f) => {
todo!()
}
Value::Double(d) => {
todo!()
}
Value::String(s) => {
todo!()
}
Value::Char(c) => {
todo!()
}
Value::Bytes(b) => {
todo!()
}
#[cfg(feature = "with-chrono")]
Value::ChronoDate(d) => {
todo!()
}
#[cfg(feature = "with-chrono")]
Value::ChronoTime(t) => {
todo!()
}
#[cfg(feature = "with-chrono")]
Value::ChronoDateTime(t) => {
todo!()
}
#[cfg(feature = "with-chrono")]
Value::ChronoDateTimeUtc(t) => {
todo!()
}
#[cfg(feature = "with-chrono")]
Value::ChronoDateTimeLocal(t) => {
todo!()
}
#[cfg(feature = "with-chrono")]
Value::ChronoDateTimeWithTimeZone(t) => {
todo!()
}
#[cfg(feature = "with-time")]
Value::TimeDate(t) => {
todo!()
}
#[cfg(feature = "with-time")]
Value::TimeTime(t) => {
todo!()
}
#[cfg(feature = "with-time")]
Value::TimeDateTime(t) => {
todo!()
}
#[cfg(feature = "with-time")]
Value::TimeDateTimeWithTimeZone(t) => {
todo!()
}
#[cfg(feature = "with-uuid")]
Value::Uuid(uuid) => {
todo!()
}
#[cfg(feature = "with-rust_decimal")]
Value::Decimal(d) => {
todo!()
}
#[cfg(feature = "with-bigdecimal")]
Value::BigDecimal(d) => {
todo!()
}
#[cfg(feature = "with-json")]
Value::Json(j) => {
todo!()
}
#[cfg(feature = "postgres-array")]
Value::Array(a) => <Option<Vec<SqlxValue>> as sqlx::Encode<
'q,
sqlx::postgres::Postgres,
>>::encode_by_ref(
&a.as_ref()
.map(|values| values.iter().map(|v| SqlxValue::from(v.clone())).collect()),
buf,
),
#[cfg(feature = "with-ipnetwork")]
Value::IpNetwork(ip) => {
todo!()
}
#[cfg(feature = "with-mac_address")]
Value::MacAddress(mac) => {
todo!()
}
}
}
}
impl<'q> sqlx::Type<sqlx::postgres::Postgres> for SqlxValue {
fn type_info() -> sqlx::postgres::PgTypeInfo {
todo!()
}
}
impl<'q> sqlx::postgres::PgHasArrayType for SqlxValue {
fn array_type_info() -> sqlx::postgres::PgTypeInfo {
todo!()
}
}
impl<'q> sqlx::IntoArguments<'q, sqlx::postgres::Postgres> for SqlxValues {
fn into_arguments(self) -> sqlx::postgres::PgArguments {
let mut args = sqlx::postgres::PgArguments::default();
for arg in self.0.into_iter() {
use sqlx::Arguments;
args.add(arg);
}
args
}
}I created a new I prefer this method to the proposed one in #225 since it allows support for user custom types. I am starting to believe that the |
|
We have another problem with the current approach. At this point we have a couple of choices:
My preference goes to 3, maybe even combined with 2. That would look similar to @ikrivosheev @nitnelave @billy1624 @tyt2y3 Let me know what you think of all this. This would be a big change but it would unblock a lot of things (including custom types). |
|
I would also incline towards 3, although can you illustrate more on the plan perhaps with some code snippets? |
|
@Sytten thank you! Can you explain (3)? Small PoC. I don't understand what you want. |
We could avoid implementing
On match arg {
Value::BoolArray(b) => {
args.add(b);
}
...
} |
|
Btw... by "custom type arrays" do you mean something like custom enum arrays? |
|
Yeah the option 2 is the easiest but does not allow for custom types. Like currently in sea-orm you have to cast the enum to string before sending it back. The issue you linked for enum array has actually been fixed I think based on the last comment. But enum is just one example you can create new types in postgres that are tuples and any array of that type will be valid (like https://stackoverflow.com/questions/41663637/insert-into-array-of-custom-type-in-postgres). We can also decide to deal with it later and use option 2 for now. I think the better solution long term is to have a Value that contains a Box of a couple of traits like encode, type info, etc. and use that to translate to the backend engine. Option 3 is similar to that in a sense that the array option of Value would contain both option of the data (like current) and an enum value of the possible types (u8, bool, etc). That would reduce the boilerplate needed since you would still have one option for all arrays. enum Type {
u8,
bool
}
struct Value {
data: Option<Box<dyn ValueTrait>>, // Or the current Value enum
type: Type
} |
|
Hey @Sytten, base on your explanation for option (3). Personally, I think this is a good direction for SeaQuery. Also, I think we should go one step further. Storing reference of P.S. That would be a complete rewrite of |
|
@Sytten in the (3) option, how can we add custom type? Something like this: enum Type {
u8,
bool,
custom(...)
}
...
? |
|
To test an idea I will try implement it for: #347 and create Draft PR. @billy1624 @Sytten ok? |
|
@billy1624 Yeah it is a big rewrite so we should really think about it before going forward with it. @ikrivosheev Thats what sqlx seems to do in their engine. A alternative would be to do something like: struct Value(Box<dyn ValueTrait>);
impl<T> ValueTrait for Option<T> {
// Here provide a method that can provide the type using T::some_method()
}That might be more elegant a solution. |
|
Hey @ikrivosheev, yea, please go ahead and conduct a small POC :D |
|
I am not sure a reference will cut it? It's nice in principle but I think we need to own the pointer otherwise it will make it harder for users to pass literal values. #[cfg(feature = "postgres")]
trait ValueTrait: postgres::ToSql {
}
#[cfg(feature = "sqlx")]
trait ValueTrait: sqlx::Encode + sqlx::TypeInfo {
}EDIT: That might not work with the generic sqlx traits as is since they need a database parameter and a lifetime so we might need 3 implementation like: #[cfg(feature = "engine-sqlx")]
#[cfg(feature = "database-postgres")]
trait ValueTrait: sqlx::Encode<'static, sqlx::postgres::Postgres> + sqlx::TypeInfo<sqlx::postgres::Postgres> {
}Unsure if the static lifetime is a good idea though, but exposing a lifetime to the trait might become soon very annoying. |
|
Regarding the redesign of I will narrow the scope and focus on SQLx only. The goal is to make By design, SQLx, needs two traits for value binding, trait SqlxPostgresValue: for<'q> sqlx::Encode<'q, sqlx::Postgres> + sqlx::Type<sqlx::Postgres> {}Then, we can add a new variant in pub enum Value {
...
#[cfg(feature = "sqlx-postgres")]
SqlxPostgres(Box<dyn SqlxPostgresValue>),
}However, Error LogFinally, we can do this for MySQL and SQLite as well: pub enum Value {
...
#[cfg(feature = "sqlx-postgres")]
SqlxPostgres(Box<dyn SqlxPostgresValue>),
#[cfg(feature = "sqlx-mysql")]
SqlxMysql(Box<dyn SqlxMysqlValue>),
#[cfg(feature = "sqlx-sqlite")]
SqlxSqlite(Box<dyn SqlxSqliteValue>),
}All in all, I think this is a good balance between "built-in types" (all existing variants in Thoughts and comments are welcome!! :D |
|
This is basically what I was proposing so I am fine with the proposal. But we do have to fix the issue of object safety for the What I am wondering is: why not convert every value to this trait. What advantage does keeping in the primitives separate from the generic use case has? It would cleanup the code a lot to just use the generic value IMO. Also I want to make sure we can support diesel types with the same pattern using the |
|
Sorry for double posting but I saw https://users.rust-lang.org/t/how-to-create-vec-for-sqlx-values/66505 which is directly related to our issue of object safety and I think the solution is one we could explore if we fail to make progress with the sqlx team. It matches well with the binders crate that we currently use too so thats good news I think. Another possible solution is detailed here https://stackoverflow.com/questions/70660975/storing-disparate-sqlx-types-in-a-vector and it is rather similar to our current situation but I am not sure It will help for arrays. |
|
Hey @Sytten, I got a draft based on the link you provided, please check https://github.com/SeaQL/sea-query/compare/WIP-value. We still have some missing pieces:
|
|
It looks good. I like that we have our own set of traits. I think we should split the trait definition from the blanket implementation so we can just create a different blanket implementation for diesel. Null is tricky if we want to keep our implementation object safe as the whole Type trait bot requiring self was in part to deal with nullable values. Basically we need to always store a value even on null. |
|
@billy1624 @Sytten @tyt2y3 well, I'm done with other features (split sea-query into crates, SqlWriter refactoring and some other small features), now i will continue to work on this feature! |
|
Good stuff, still a few things to resolve before we can finalize this but you have good stuff to read above. |
|
Hey @ikrivosheev, the context of why we need to wrap every value in |
Motivation
Postgres arrays are not supported in the query binders (see #275) or drivers.
Proposed Solutions
We should implement the conversion from
Vec<sea-query::Value>to someVec<sqlx::Encode>insea-query-binders/src/sqlx_postgres.rs. For this, we'll need to ensure that all values are of the same type, and do a second level of dispatch based on the value types to convert them to the right type.The text was updated successfully, but these errors were encountered: