-
-
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
add select_by #2367
add select_by #2367
Conversation
diesel/src/deserialize.rs
Outdated
/// # Ok(()) | ||
/// # } | ||
/// ``` | ||
pub trait TableQueryable { |
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, the best name will be SelectResult
or even Selectable
, because as you noted, implementors of this trait not necessary represents a table
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 also think TableQueryable is not a good name.
I think Selectable
is better, would there be any conflict (or conflict in the future)?
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 agree that this should have a better name, Selectable
is not bad, but I'm not 100% sold to it yet.
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.
Changed to QueryableByColumn
.
If there's some better name, welcome to propose.
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 don't like that name because it's completely obscure. What do ByColumn
mean? Looks as if usual Queryable
doesn't query columns.
Maybe AutoQueryable
? Auto
means, that struct fields automatically maps to table columns. Although there may be other, unwanted connotations. Selectable
in this sense a much better. It is similar to Queryable
, but different.
One other variant is Mapped
, that means that struct is mapped to some table or view.
Or even just TableOrView
.
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 like QueryableByColumn
either, Selectable
sounds better for me.
That said: This is a discussion that does not really matter till the actual feature implementation is finished.
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 is not "Auto", the column name could be specified by #[column_name = "id"]
while Mapped
and TableOrView
seems a little unrelated.
I think Queryable
do not query columns, they query index. Queryable
query index, QueryableByName
query names, and QueryableByColumn
query columns. (but we would use term "SelectExpression" instead of "Column", so it seems Selectable
more close to SelectExpression
)
While I worry about Selectable
would be confused with SelectableExpression
. And it might also have more opportunity to conflict with other top level ident.
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.
Anyway, I'd like to change the trait name and change Columns
to SelectExpression
at the last second, I don't have a refactor tools and it is much easier to have QueryableByColumn
to be sed
than Selectable
:)
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.
Thanks for working on this 👍
I've done a quick review of the change and added notes as a went through the code. Please do not consider this review to be fully complete in the sense that I will probably add comments in other places later as well.
Beside of that I also would like to see the following things added:
- A bunch of test cases in
diesel_tests
, basically checking that all thoseQueryDsl
methods are implemented correctly - A changelog entry
- Improved/fixed documentation.
diesel/src/deserialize.rs
Outdated
/// # Ok(()) | ||
/// # } | ||
/// ``` | ||
pub trait TableQueryable { |
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 agree that this should have a better name, Selectable
is not bad, but I'm not 100% sold to it yet.
Is there any suggestion for the trait/derive name instead of |
With #2182 merged now we can avoid much of the code duplication here. The new basic idea is as following:
This should reduce the complete code to something like pub trait Selectable {
type Selection: crate::expression::Expression;
}
pub struct SelectBy<ST>(std::marker::PhantomData<ST>);
impl<ST> TypedExpressionType for SelectBy<ST> {}
impl<ST, DB> QueryMetadata<SelectBy<ST>> for DB
where
DB: Backend,
{
fn row_metadata(_: &Self::MetadataLookup) -> Option<Vec<Self::TypeMetadata>> {
None
}
}
impl<Q, DB, ST> FromSqlRow<SelectBy<Q>, DB> for Q
where
ST: crate::expression::Expression,
DB: Backend,
Q: Selectable<Selection = ST>,
Q: FromSqlRow<SqlTypeOf<ST>, DB>,
{
fn build_from_row<'a>(row: &impl Row<'a, DB>) -> Result<Self> {
<Self as FromSqlRow<SqlTypeOf<ST>, DB>>::build_from_row(row)
}
}
use crate::expression::SelectableExpression;
use crate::query_builder::select_clause::*;
impl<QS, S> SelectClauseExpression<QS> for SelectBy<S>
where
S: Selectable,
S::Selection: SelectableExpression<QS>,
{
type Selection = S::Selection;
type SelectClauseSqlType = SqlTypeOf<S::Selection>;
}
This should greatly simplify the implementation and remove the blockers from my list above. |
Thanks for response and not forget about this PR. What about GroupBy? I think the only blocker was that you said you'd like to rethink
These are still not answered. |
I'm not saying I'm giving this PR up. But I'd like to do nothing until things went clear. |
I can fully understand that it is really frustrating that this PR is not merged yet, but as this is a really large change I do want to make sure that we get it somewhat right before merging it. In the end it's up to us diesel maintainers to maintain and live with all code we have merged. So better make sure those code is fine, otherwise we run into large trouble pretty soon (and probably will lose any interest in continue maintaining diesel). And yes working on OpenSource can sometimes be really frustrating. (Not only as contributor, but also as maintainer. Just check the issue tracker about what some comments request from us, because someone cannot use diesel for their business case...) Additionally this feature is not something I personally need, therefore it's something where I accept contributions but will not spend much time on my own to figure out remaining open points. In the end my day has only a fixed amount of time and I cannot do everything. (And I'm not willing to spend all my free time on maintaining a open source project for other people. It's more like I fix the things I personally care about and spend some time to help/guide other people to fix/implement their features.) Therefore the list of open questions, as nobody has spend time to work on those it seems like there is not that much interest in this feature. As for the concrete open points:
This remains a open question. In the end it just needs someone who sits down tries out a few possibilities and provide a summary write up what worked well and what not. In the end this is a cosmetic small scale change, which would not change much of the underlying implementation. Just trying to get a nice API.
As mentioned here: I would like to see and compile_fail test for some trivial case. Those needs to be written.
#2182 Allows to remove the code duplication. As mentioned here we can now implement this feature by "just" having a custom select clause instead. This should allow to remove basically all of This was not something that was clear in the beginning of this PR, but some of my motivation while implementing #2182 was to simplify approaches like this one and remove the code duplication here.
To write that again: I'm willing to help here with advice + code review as far as I find some spare time, but I do not have the time capacity or motivation to work on this feature by myself. If someone is interested in getting this merged they need to spend time and address the points above. |
I think we doesn't need |
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.
Thanks for picking this up again. It looks now nearly finished. I left a bunch of small stylistic comments, otherwise this is fine from my side.
What I would like to see are 2 or 3 simple compile fail tests showing that this feature correctly rejects malformed select clauses (something like table appears not in from clause + maybe mixed aggregates).
SE: Expression, | ||
Selection: Selectable<Expression = SE>, | ||
Self: SelectDsl<SE>, | ||
<Self as SelectDsl<SE>>::Output: SelectByQuery<Expression = SE>, |
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 why the SelectByQuery
bound here is required. Instead of that bound there should be some bounds along the lines of SelectBy<Selection>: SelectableExpression<F>
to verify that the select clause is fine for the given from clause.
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 just what SelectDsl write.
Co-authored-by: Georg Semmler <Georg_semmler_05@web.de>
No, it doesn't work. #[derive(Selectable)]
#[table_name = "users"]
struct Id {
id: i32
} Now the problem is, there's no way to make
impl<T: Query> AsQuery for T {
type SqlType = <Self as Query>::SqlType;
type Query = Self;
fn as_query(self) -> Self::Query {
self
}
}
|
@clouds56 I've put together a minimal version that works here. That obviously needs some cleanup + refactoring + finishing the implementation, but I think the basic idea should be clear. Edit: Technically the changes to |
That's an interesting point to introduce constraints with |
@clouds56 Thanks for updating the PR 👍 I'm quite occupied with work currently, but will try to find some time for looking into that. |
to support mixed typed/untyped queries
@clouds56 I've found some time to look at this. I think it should be fixed by the impl added with my last commit. |
/// let all_users = users.load::<(i32, String)>(&connection)?; | ||
/// assert_eq!(vec![(1, String::from("Sean")), (2, String::from("Tess"))], all_users); | ||
/// | ||
/// let all_names = users.select_by::<UserName>().load::<UserName>(&connection)?; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
fixes #2037
Selectable::Expression
GroupBy
Join
/InternalJoinDsl
AsQuery
DistinctOnDsl
,ToInnerJoin