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

issues-336 Array support #413

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ikrivosheev
Copy link
Member

Comment on lines 32 to 33
#[derive(Debug, Clone)]
pub struct Value(Rc<Box<dyn ValueTrait>>);
Copy link
Member

Choose a reason for hiding this comment

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

Hey @ikrivosheev, thanks for the draft!!

One question come up in my mind. For sea-query-binder and sea-query-driver, how do we bind the value on db driver? Because we can't figure out what's the type that implemented ValueTrait without the help of downcast and Any trait.

Copy link
Member

Choose a reason for hiding this comment

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

@tyt2y3 ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

@billy1624 this is the biggest problem... Another idea is move sqlx and driver into sea-query

Copy link
Member Author

Choose a reason for hiding this comment

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

@Sytten ideas?

@billy1624 I can make two PR with with downcast and with move sqlx and driver into sea-query.

Copy link
Member

Choose a reason for hiding this comment

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

The chain of pings loll

Copy link
Member Author

Choose a reason for hiding this comment

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

@tyt2y3 I think, I undestand!

Copy link
Member

@tyt2y3 tyt2y3 Aug 16, 2022

Choose a reason for hiding this comment

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

keeping the enum when you have the boxed trait

I wanted to pass-around primitives (and to some extend anything less than 16 bytes and impl Copy) on the stack, albeit they end up in a Values Vec.

Copy link
Member

Choose a reason for hiding this comment

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

I imagine it's like

// Inside the root of sea-query

trait ValueTrait {}

enum SeaValue {
    I32(i32),
    Object(Box<dyn ValueTrait>),
}

impl ValueTrait for SeaValue {}

// Inside sea-orm-binder

struct SqlxMySqlValue<T>(T)
where
    T: ValueTrait + for<'q> Encode<'q, MySql>;

impl<'p, T> Encode<'p, MySql> for SqlxMySqlValue<T>
where
    T: ValueTrait + for<'q> Encode<'q, MySql>,
{
    fn encode_by_ref(
        &self,
        buf: &mut <MySql as sqlx::database::HasArguments<'_>>::ArgumentBuffer,
    ) -> sqlx::encode::IsNull {
        self.0.encode_by_ref(buf)
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

And, maybe...

trait ValueTrait {}

enum SeaValue {
    I32(i32),
    Object(Box<dyn ValueTrait>),
}

impl ValueTrait for SeaValue {}

impl<'p> Encode<'p, MySql> for dyn ValueTrait
where
    Self: for<'q> Encode<'q, MySql>,
{
    fn encode_by_ref(
        &self,
        buf: &mut <MySql as sqlx::database::HasArguments<'p>>::ArgumentBuffer,
    ) -> sqlx::encode::IsNull {
        self.encode_by_ref(buf)
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@billy1624 it seems like it won't work...

// Inside the root of sea-query

trait ValueTrait {}

enum Value {
    I32(i32),
    Object(Box<dyn ValueTrait>),
}

impl ValueTrait for Value {}

// Inside sea-query-rusqlite

#[derive(Clone, Debug)]
struct RusqliteValue<T>(T)
where
    T: ValueTrait + ToSql;

impl<T> ToSql for RusqliteValue<T>
where
    T: ValueTrait + ToSql,
{
    fn to_sql(&self) -> Result<ToSqlOutput<'_>> {
        self.0.to_sql()
    }
}

#[derive(Clone, Debug)]
pub struct RusqliteValues<T>(pub Vec<RusqliteValue<T>>);  // <--- I cannot put here enum Value because it does not impl ToSql and I cannot make this parameter generic and I cannot impl ToSql for Value because it from other trait

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 16, 2022

Perhaps we can start from removing the object variants from Value (while keeping the primitive variants)?

@billy1624
Copy link
Member

The requirements:

  1. No downcast
  2. Keep SeaQuery clean, outsource heavy lifting of value binding to binder
  3. Include binder in sea-query's dependency but do not turn on the features by default
  4. It could be Value in sea-query, ValueTrait in binder
  5. Could we enable multiple backend support in an additive compatible way? I.e. if one downstream turned on SQLx and another turn on postgres. The ? cargo syntax should be helpful

@ikrivosheev ikrivosheev force-pushed the feature/issues-336_array_support_v2 branch from fb44aa9 to e98f4eb Compare October 8, 2022 20:53
@ikrivosheev
Copy link
Member Author

@billy1624 @tyt2y3 , hello! I have a bad surprise. trait ToSql from postgres-types crate need: Self: Sized, but I cannot make ValueTrait: Sized, because I need dyn ValueTrait

@ikrivosheev
Copy link
Member Author

It looks like instead of create our own ValueTrait make all types, which hold Value should be generic...

@ikrivosheev
Copy link
Member Author

ikrivosheev commented Oct 8, 2022

Hmmm another sought... Black magic with unsafe. This should work. I can store row pointer to data and then transmute it to value and type get from enum.

Example: https://github.com/Diggsey/ijson/

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 9, 2022

May be we should postpone ValueTrait to after 0.27, and trying to get array working with sqlx-postgres for now

@ikrivosheev
Copy link
Member Author

ikrivosheev commented Oct 9, 2022

Ah, one more problem... I try something like this:

pub trait Sqlx<'q, T: Database>: Encode<'q, T> + Type<T> {}

pub trait ValueTrait
where
    Self: Debug,
{
    fn to_sql_string(&self) -> String;

    #[cfg(feature = "with-postgres")]
    fn as_postgres_to_sql(&self) -> &(dyn PostgresToSql + Sync + Send);

    #[cfg(feature = "sqlx-mysql")]
    fn as_sqlx_mysql<'q>(&'q self) -> &(dyn Sqlx<'q, MySql>);

    #[cfg(feature = "sqlx-postgres")]
    fn as_sqlx_postgres<'q>(&'q self) -> &(dyn Sqlx<'q, Postgres>);

    #[cfg(feature = "sqlx-sqlite")]
    fn as_sqlx_sqlite<'q>(&'q self) -> &(dyn Sqlx<'q, Sqlite>);
}

#[derive(Debug, Clone)]
pub enum Value {
    Bool(bool),
    TinyInt(i8),
    SmallInt(i16),
    Int(i32),
    BigInt(i64),
    TinyUnsigned(u8),
    SmallUnsigned(u16),
    Unsigned(u32),
    BigUnsigned(u64),
    Float(f32),
    Double(f64),
    #[cfg(feature = "thread-safe")]
    Object(SeaRc<Box<dyn ValueTrait + Sync + Send>>),
    #[cfg(not(feature = "thread-safe"))]
    Object(SeaRc<Box<dyn ValueTrait>>),
}

impl ValueTrait for Value {...}

Problems:

  1. I cannot combine rusqlite and sqlx in one crate (failed to select a version for libsqlite3-sys)
  2. I cannot make &dyn Sqlx because Type<T> - Sqlx cannot be made into an object` - Make sqlx::Type object safe by taking &self launchbadge/sqlx#1461
  3. sea-query-postgres throw compile error: the to_sql method cannot be invoked on a trait object`

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 11, 2022

1: I think we won't combine them
2: we have to create our own trait based on that but is object safe

@billy1624
Copy link
Member

I guess we can close this?

@ikrivosheev
Copy link
Member Author

I guess we can close this?

@billy1624 I think, no, we cannot... I will continue work on it.

@billy1624
Copy link
Member

Alright, a trait based Value with array support would be cool. And a huge step forward :)

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

Successfully merging this pull request may close these issues.

Query binders: Add support for postgres arrays
4 participants