Explain that not only the order of fields needed

[pickfire]

Queryable need to match all the fields, not just the order but the number
in some cases (I don't know what case).

@weiznich @Mingun #2627 (comment)

I am confused myself about the behavior too, I only know in some cases it needs to be the same, after I read the docs I became more confused (since it works even when the number of fields is not the same but at least the order is the same), I tried my best to reduce the confusion saying that all the items needs to be the same.

At first I even thought this will do a smart job extracting the fields needed, like the struct being part of the schema.

[Ten0]

I'm not sure what cases you're referring to that don't respect that rule.
How about:

Suggested change

	/// no effect. In some cases, all the fields in the struct (including the order)
	/// must be exactly the same (like `Eq`) as the fields in the query.**
	/// no effect.** There should always be as many fields in your structure as
	/// elements in the tuple used as in the `select` part of the query. This also
	/// applies if the `select` is implicit.

[pickfire]

In some cases like when join it needs the whole items, not as many fields but all the fields. But I am not sure if there are any other cases besides that, I only encountered it once (I rarely use diesel). So this changes doesn't seemed to improve a lot from what is there currently (I find it a bit incorrect from what I know).

[Ten0]

not as many fields but all the fields

I don't understand. Can you explain along with an example of a code that exhibits that behavior?

[pickfire]

#2627 (comment)

[weiznich]

@pickfire As already explained there: This is not related to joins. The behaviour is the same for every query. To word it really simply: Each query has a at compile time know SQL type, which is based on the expressions in your select clause (note that expressions are not necessarily columns…). Let's name that ST. If you now call .load() (or a similar method) the result struct needs to implement Queryable<ST, _> for your return type. The #[derive(Queryable)] now implements Queryable by just using mapping each return field by index to one of your struct fields, so that ST corresponds to each rust type. There is really no magic going on here and the behaviour is just the same for all queries.

[weiznich]

Ok, the whole discussion/problem seems to be more about what's the default select clause in which case, then how #[derive(Queryable)] work.
For me it does not seem like a good idea to document that all on fn load() is this function is not related to constructing the select clause.
There are exactly three cases for how the select clause looks like:

• A user passes an explicit select clause via select. It think the examples and the explanation there cover this case quite well. Otherwise that's the right location to improve documentation about this case.
• A user does not call .select(…) and does not perform a join. This is the case with the weakest current documentation, as this is done implicitly and there is no "right" place to document this. It is somewhat covered in the Getting Started but that does not go in depth there.
• A user does not call .select(…) but performs one or more joins. This case is documented via inner_join (All other join methods like this, it's basically the same for them). Now this documentation is quite detailed about the return types, but could mentioned a bit more what's the behaviour if there is already an existing select clause and how the new default select clause is constructed.

The right place to cover all that would probably be to write a All about selects guide (See #1108), but at least I do not have the time to do that.

I believe the usual case is that one can load a single struct with implicit select if there are less fields in the struct but not in the case of join. Maybe we need to explain the need to have explicit select in some parts?

Again: The rules for calling load() are the same, regardless if there is an join or not. There is no such thing like a implicit select that just drops fields and that's an explicit design decision of diesel. The field could and structure needs to match with that data structure you are loading data into. (At least as long as you use the derive…)

maybe this should be explained in load in which an explicit select is needed if a join is used, since it became a tuple.

It's perfectly fine to use load() on a joined query without calling select.

[Ten0]

EDIT: Oh, previous answer already covered that. I missed it when I posted this.

I believe the usual case is that one can load a single struct with implicit select if there are less fields in the struct but not in the case of join.

I don't think there is such a case even when there are no joins: it's always mandatory when deriving Queryable that there are as many fields in your structure as elements in the tuple used as in the 'select' part of the query, even with an implicit select.

[pickfire]

Thanks for clarifying, now I think I understand more.

Is there a way to prevent footguns like these where we can check the original schema to see if it matches the struct? These seemed like implicit knowledge which the users needs to check themselves which make it a potential footgun, I think it would be best to have these enforced during #[derive(Queryable)] but not sure if that's possible.

[weiznich]

The main point of having something like Queryable is that it does not require the schema to work, because otherwise you wouldn't be able to represent the result of a more complex query with it. How would you handle the following SQL with only checking the schema? SELECT departments.id, 42, max(employees.salary) FROM departments INNER JOIN employees ON departments.id = employees.department GROUP BY departments.id ? Such query does not necessarily contain references to only one table, could mix columns through arbitrarily complex expressions and can even contain expressions without reference to columns. You cannot check that with "the schema".

[pickfire]

I believe this is resolved right?

[pickfire]

I have updated it to say that all fields needs to be the same.

[pickfire]

@weiznich Do we still need the FIXME in QueryDsl::select?

[weiznich]

Thanks for the update. This sounds much better now 👍
I've left a few comments with some minor formulation improvements + 2 places where the change incorrectly mentions that a #[table_name] attribute is allowed while it isn't.
If that's changed I'm fine with merging this.