-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow to implement a dynamic select clause feature in #2182
Allow to implement a dynamic select clause feature in #2182
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely clear on what this feature is trying to do, so I'm not really comfortable signing off on it. That said, the code itself generally looks fine. The count methods are more clearly reasonable to me, but I would rename them to field_count
, since an item in the result may not map to a column. The name functions are in my opinion misleading, and encourage a common misconception about SQL which is not true.
To motivate this a bit more: If you currently construct a select clause you need to know how many fields you select at compile time, because result sets are entirely represented at tuples at compile time. (Thanks to #2134 you only need to know the number of arguments not the actual type, at least for postgres). Now there are certain situations where those information are not available at compile time, for example if your application tries to infer the schema at run time. In such situations it would be handy to have a method that gives you some kind of iterator over fields in a row at runtime. I think we've agreed some while ago that the user facing part of this feature belongs to |
bc8acce
to
ff4f541
Compare
}; | ||
unsafe { | ||
Some(CStr::from_ptr(field.name).to_str().expect( | ||
"Diesel assumes that your mysql database uses the \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be to use the encoding
crate and get message from its real encoding? Or may be it is possibly specify desired encoding per-query basis in MySQL API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every other impl that deals with stings and is part of the diesel mysql backend assumes the same. It makes no sense to change that in this one case. Changing all impls is clearly out of scope of this PR. If you are interested in this, go for it. I'm not willing to invest time in changing this in a foreseeable future, because that's something that has no value for me.
Hi, dynamic selects would be a great addition to the framework, and code seems good for me. Not big changes, just exposing some extra information. |
@Razican Nerveless I would like to have some discussion with the @diesel-rs/contributors about the general design of this feature, so just merging it is not an option. |
25233a5
to
5a93ca6
Compare
I had some time to think about possible solutions to support that for more than the postgresql backend. So here a short mind dump of the remaining open questions for this feature: (cc @sgrif would like to hear your opinion an this 😉) |
I think your comment makes sense, but I still don't think it makes sense for |
5a93ca6
to
a2fa457
Compare
Just to add my $0.02: I am really happy to see things moving in the direction of dynamic select lists; I've been meaning to ask about them in gitter forever, but obviously haven't. I have exactly this situation: the schema is only known at runtime (user-supplied) Currently, I work around this restriction by calling |
For now blocked on #2290 |
0744b7e
to
c8bfe3b
Compare
/// Raw sqlite value as received from the database | ||
/// | ||
/// Use existing `FromSql` implementations to convert this into | ||
/// rust values: | ||
#[allow(missing_debug_implementations, missing_copy_implementations)] | ||
pub struct SqliteValue { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general note: I'm not sure if this is sound in combination with the pointer casting in line 28, as this relays on the ABI representation. Maybe we should try to add something like #[repr(transparent)]
here?
@diesel-rs/reviewers I would like to proceed on this one. If someone finds some time a review would be appreciated (especially about the API design and the unsafe code usage). @Ten0 I think I will find some time to work on this in the next weeks. Would be great to get some inputs regarding to #2275. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is technically solid. I don't have any opinions on the change and design though.
9433438
to
71c378a
Compare
Regarding the "Remove Given that
Regarding these 3 points (and in particular the first one), what do you think of turning the Unrelated to that, I beleive |
@Ten0 OK I took some time to understand the nullable boolean problem uncovered by the CI. Seems like this fix will not work in that way. |
Ah yes, good old 26720d7 / #104 hurting again (I say again because #1459 (comment)). Thinking about #104 more seriously than I did last time I saw it (which was my first interaction with Diesel), it seems we could work out a different way to solve #104 (in what Sean called the "real answer") by, instead of using a Once that is done, hopefully there is no more blocker to your initial approach to deserializing |
0fd1453
to
c7545bc
Compare
f8baa44
to
137b3e9
Compare
@diesel-rs/reviewers I think this is now ready for a final review (assuming that CI now passes, otherwise I will fix the CI and then it's ready). If there are now comments in the next 7 days I will merge this PR. cc @adwhit as owner of |
91c6757
to
197a51f
Compare
(Sorry for the really large diff but this touches nearly all parts of diesel) This commit does in two things: * Refactor our handling of nullable values. Instead of having a marker trait for non nullable sql types we now indicate if a sql type is nullable by using a associated type on a new fundamental trait named SqlType. This allows us to reason if an type is nullable or not in a much precise way without running in conflicting implementation issues. This allows us to address diesel-rs#104 in a much more fundamental way. (The correct way as mentioned there by sgrif). * Refactor our handling of typed and untyped sql fragments. Instead of having two separate code paths for `Queryable` (sql types known) and `QueryableByName` (sql types not known) we now have only one code path and indicate if a query is typed or untyped as part of the expression sql type. This is required to address diesel-rs#2150. `Queryable` and `QueryableByName` now simple . Additionally we separate the static size component of `FromSqlRow` to allow dynamically sized rows there (but only at the last position for tuple impls.) I should probably have implement those changes in different commits but as both changes basically requiring touching most of our code base this would have required much more work... Both changes are theoretically big major breaking changes. For application code I expect the actual breakage to be minimal, see the required changes in `diesel_tests` and `diesel_cli` for examples. For highly generic code I would expect quite a few required changes. Additionally there are a few small drive by fixes. Fixes diesel-rs#104 Fixes diesel-rs#2274 Fixes diesel-rs#2161
197a51f
to
ee470bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as soon as I had hoped, but I could finally take the time to analyze this! o/
This is great! There are tons of improvements everywhere which overall make the code conceptually simpler while fixing issues and introducing new functionality.
I have a few questions/remarks at the implementation level, but haven't found any issue with the design. 👍
|
||
/// Returns a wrapping row that allows only to access fields, where the index is part of | ||
/// the provided range. | ||
#[doc(hidden)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this is #[doc(hidden)]
. Isn't there a scenario where someone implementing a dynamic query would ideally use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see how this would be relevant for dynamic queries. They normally should use RowIndex<&str>
or just load all columns that are queried. In my view this function is just a internal helper to implement FromStaticSqlRow
for recursive tuples by allowing to skip the corresponding number of fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could imagine a scenario where there is still some form of known hierarchical structure, but user-defined and unknown at compile time which could use this when building it through a recursive function.
What would be the downside of having it public? (with corresponding documentation explaining this could only be useful when implementing custom parsing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can make this function public at any later point if someone comes up with a concrete use case.
if self.0.name.is_null() { | ||
None | ||
} else { | ||
unsafe { CStr::from_ptr(self.0.name).to_str().ok() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be different from the others (which expect
). Is that because Mysql does not guarantee that they will be UTF8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's different in that way that we have some way to handle non UTF-8
field names here, we can just return None
in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. On the other backends, we decided to not silent-fail on this and expect
, because they should never return non utf-8.
On this backend it seems we decided to return them as non-named fields.
Is it because in the way we're using Mysql now it is expected behaviour that it might return non-utf-8 values?
Oh well nevermind I guess that's probably the correct solution if the previous correct solution was what was previously implemented, that is c_name.to_str().unwrap_or_default()
^^
fn from_nullable_sql(bytes: Option<backend::RawValue<DB>>) -> Result<Self> { | ||
match bytes { | ||
Some(bytes) => Self::from_sql(bytes), | ||
None => Err(Box::new(crate::result::UnexpectedNullError)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have chosen to not apply the suggestion at #2182 (comment).
I'd like to understand what makes the current solution better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in my opinion the simpler solution. As for the additional allocation + dynamic dispatch: I'm not sure if the compiler optimizes them away or not but at least in my short benchmark I haven't seen any impact of this. Therefore I've the opinion that we should just choose the simplest option here and not try to optimize things that are not worth being optimized. If someone provides some benchmark showing that this assumption is wrong I will change the implementation 😉
As for making it clear that this impl checks for crate::result::UnexpectedNullError
: I changed FromSql
in such a way that basically all impls will return this error if they encounter an unexpected null value, only impl that choose to provide their own from_nullable_sql
impl have to deal with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it seems I have indeed underestimated the complexity of that change and overestimated the ease of use of the corresponding interface (which cannot be as nice as I thought due to conflicting impls issues).
Thanks! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The complexity of the change it self is not so much the problem here, it's the potential impact of the change that is in my opinion the problem. If we introduce some enum like
enum DeserialisationError {
UnexpectedNullError,
Custom(Box<dyn Error + Send + Sync>)
}
that would change the return type of every function that uses diesel::deserialisation::Result
which is a large impact breaking change in my opinion. We can do such changes with diesel 2.0, but I think we should only make major breaking changes where it is really required. At least for me that seems not to be the case here as I cannot measure a negative performance impact of handling null values this way + having a default FromSql::from_nullable_sql
method should make it clear how to correctly "handle" unexpected null values.
I have executed the benchmarks myself, and while I was having some inconsistencies in different runs, I can confirm that I didn't see any significant performance regression. |
Co-authored-by: Ten0 <[email protected]>
b95b837
to
c4d15b0
Compare
c4d15b0
to
4e69728
Compare
diesel_dynamic_query
This commit contains the following changes:
select clause variant based on a vector instead of tuples like the
existing ones. This allows to construct dynamically sized select
clauses
Row
traits to which allows to get moreinformation (current column name and column count) of the result
set. This is required to implement compatible result types for
dynamically sized select clauses