Include from_sql_function in Diesel

[Ofenhed]

As per recommendation in #815 I'm suggesting adding a separate macro for SELECT ... FROM function.

I suggest that the macro from_sql_function! is moved almost as is (depending on what works and what doesn't) to the diesel package. One good change before it's moved (in my opinion) is to make it match the current sql_function! syntax.

Suggested syntax would be:

 from_sql_function! {
    #[sql_name = "some_function"]
    fn my_sql_function(some_var: i32, some_other_var: i32) {
        #[sql_name = "id"]
        some_id -> i32;
        title -> String;
    }
}

Just to make sure it gets into the package I would suggest skipping the function meta for now and just add fn before the function name and otherwise use it as is, since that syntactically allows the addition of meta tags later. Apparently the existing from_sql_function is not general enough to be production worthy. I'm afraid I can't speak for what's there, what's missing and what's too specific, so I guess that would be part of this discussion.

My use case would be a query such as SELECT [...] FROM my_function(parameter) INNER JOIN [...] INNER JOIN [...].

I'm not able to use big copy-pasted (and therefore unmaintained) macros in the project I work on, so having it shipped with the official diesel package would be an absolute game changer for me. Apart from my purely egoistic reasons, I think this ability is critical to database performance since the alternatives right now (as far as I can tell) are to lose type safety or to run multiple queries. I think it should be part of diesel because sql_function! is included and this is a very similar macro with a use case that does not overlap. I also think it should be included in the main package since Diesel is a complex system with many macros and types, and having it as part of the main package release test process makes it less likely to break.

There are no downsides to adding this, as far as I can tell, besides the fact that this is now part of Diesel and may have to be maintained if the underlying type system is changed in the future.

Any thoughts on this suggestion?

[weiznich]

I suggest that the macro from_sql_function! is moved almost as is (depending on what works and what doesn't) to the diesel package. One good change before it's moved (in my opinion) is to make it match the current sql_function! syntax.

The first thing to make this proposal actionable is to check how that macro works and what does not work. After that this can be a starting point for a discussion about how the API could look like and how the corresponding implementation should be designed. (As the original author of this macro I can tell you that is basically a hack for using this macro in a certain context. That means that macro makes different choices that it would be sensible for something that is included in diesel itself. For example a similar macro in diesel should not implement Table for the function struct, as this is not a table).

I also think it should be included in the main package since Diesel is a complex system with many macros and types, and having it as part of the main package release test process makes it less likely to break.

Diesel has a clearly defined public API + is following semantic version. We've put a lot of work in the past in not doing major version bumps with any release. Additionally the upcoming 2.0 release is expected to have only minor impact on crates actually using diesel. This means everything implemented in a third party crate is fine as long as it only uses the public API. In fact a third party crate is probably better maintainable because of this, then having the implementation in diesel itself.

There are no downsides to adding this, as far as I can tell, besides the fact that this is now part of Diesel and may have to be maintained if the underlying type system is changed in the future.

At least for me this is a quite large downside. I'm currently more or less the only maintainer of diesel and I do only work sometimes in my free time on this project. Given this situation adding a large new API to diesel is something that I see critical, as long as it is possible to implement basically the same in a third party crate. I totally understand that you are not willing to use a unmaintained macro in your project, but adding the same macro to diesel itself, only shifts the maintenance burden to me and I'm more than busy with maintaining what's diesel today without further additions. This does not mean that I'm totally opposed in including something like that in diesel itself, but I would expect that someone that wants to have this in diesel spends the necessary work on designing , implementing, documenting + testing the corresponding API in such a way that it is in line with all the other parts in diesel.

[Ofenhed]

I have actually started reading the macros for table!, sql_function! and from_sql_function!. I'm currently thinking about trying an approach that reuses the private underlying macros in table! with the following syntax:

sql_function! {
   #[meta]
  fn function_name(arg1: Arg1Type) {
    #[meta]
    ret1 -> Int,
    #[meta]
    txt -> Text,
  }
}

The #[meta] are just placeholders for the optional meta parameters.

The caveats I know of so far is that the impl Table would have PrimaryKey = () for tables returned from a function. Without having had the time to test I would assume that this makes it so that this table cannot be joined into other queries. Do you see something inherently stupid in that approach? In the Postgresql definition of a function it actually returns a table, so as far as I can tell there's no semantic issue (at least for postgres) to use the Table trait for this.

The implementation of my suggestion would require a variable added to the inner state of the macros and some static_conds introduced in sql_function! and in the private functions behind table!. My (maybe naive) hope is that this would add this functionality without cluttering the macros or make them harder to maintain. Would you be open to this approach? If so, I'd be interested in taking a stab at it.

[weiznich]

The caveats I know of so far is that the impl Table would have PrimaryKey = () for tables returned from a function. Without having had the time to test I would assume that this makes it so that this table cannot be joined into other queries. Do you see something inherently stupid in that approach? In the Postgresql definition of a function it actually returns a table, so as far as I can tell there's no semantic issue (at least for postgres) to use the Table trait for this.

To be clear the underlying quest ast node generated by a such a hypothetical macro should not implement Table in any case, as it is no table. It has no primary key and so on.

The implementation of my suggestion would require a variable added to the inner state of the macros and some static_conds introduced in sql_function! and in the private functions behind table!. My (maybe naive) hope is that this would add this functionality without cluttering the macros or make them harder to maintain. Would you be open to this approach? If so, I'd be interested in taking a stab at it.

I believe it's not as easy as just combine existing pieces, for the following reasons:

• The type returned by the function is no table, so implementing Table is a no go. Which means you cannot large parts of the existing table! infrastructure.
• table! is written as old style macro_rules! macro, while sql_function! is a new style function like proc macro. You cannot easily pass state (or anything) between those two.

Before starting with a concrete implementation I would like to discuss the public API a bit further. I feel that the variant you've proposed above does not really fit into diesel as it is some strange hybrid between table! and sql_function!. sql_function! strictly follows normal rust function definitions while, table! is a completely custom syntax. Mixing both will only lead to confusion in my opinion. I nee the following options for such a API:

• Have another distinct macro that could freely design it syntax. This should likely follow the table! syntax.
• Somehow make the QuerySource impl for sql functions dependent on the return type. If that's a t.ype that implements QuerySource this should be also done for the whole function. This allows us to just separately define the corresponding return type and leaf all other API's as they are. The definition of that return type should then happen separately in a distinct macro. (That's probably the preferred variant from my point of view.)

[Ofenhed]

So how would you feel about a sql_from_function! that simply replace the primary key from table! with the input types of the function, and make that field non-optional? They should have pretty much the exact same requirements/functions in regard to the database.

Looking at the type system questions surrounding Query source, do you have any hints on what I should look at to help with that?

Note: I have worked around my issue by using a sql_query!, but I still think this would be a great addition to Diesel, so I still want to help.

[weiznich]

So how would you feel about a sql_from_function! that simply replace the primary key from table! with the input types of the function, and make that field non-optional? They should have pretty much the exact same requirements/functions in regard to the database.

I don't think that would work. For example the following SQL function does not even contain some of the arguments passed to the function as "column" (This is obviously just some dummy example…):

CREATE OR REPLACE FUNCTION user_with_id_plus_const (constant INTEGER)
RETURNS TABLE(user_id INTEGER, username TEXT) AS $$
DECLARE
result record;
BEGIN
    RETURN QUERY SELECT user_id + constant as user_id, username FROM users;
END;
$$ LANGUAGE plpgsql

Fundamentally speaking I think we should not try to make such a macro return something that implements Table as it isn't a table. Diesel has a distinct concept of QuerySource here already, which is basically a super trait of Table. That means all tables are QuerySource's but not every QuerySource is a table. In my opinion these functions fit quite well the general description of QuerySource as we can provide a FromClause and a DefaultSelection for those functions, but not that of Table as we don't have a primary key for example.

Now the way forward here in my opinion would be something like this:

1. Write a query dsl node for a single specific sql function that can be used as from clause. This example should concentrate on only implementing traits that are easily implementable, like QuerySource and avoid traits like Table where we would need to find some sort of compromises.
2. Using the example implementation from the first step we should now toy around with that function and see what's actually possible to do and what's not. This involves just testing out various parts of QueryDsl and so and. In the end just write some queries and see which ones are possible and which not. This should involve things like select, join, filter and so on. We should also check which parts of QueryDsl should not be available for these kind of from clauses and then check that they do not compile.
3. At this point we should have a clear idea which traits should be implemented what way. That means now is the point to talk about the actual design of the API exposing this feature. In the end this is likely a macro similar to the existing from_sql_function! macro linked above.

I know that all of this is probably quite a bit of work and requires a quite good understanding of diesels internals. While I'm probably interested in having this feature in diesel itself, I personally do not have the time to experiment and write down a concrete implementation and API proposal here. I'm fine if someone else picks this up and starts working on this and I will likely answer questions about diesels internals, but I cannot provide a step by step guide for this.