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

The WITH clause #222

Closed
neevek opened this issue Dec 14, 2018 · 19 comments · Fixed by #1124
Closed

The WITH clause #222

neevek opened this issue Dec 14, 2018 · 19 comments · Fixed by #1124

Comments

@neevek
Copy link

neevek commented Dec 14, 2018

This library has very cool API design, thank you @fnc12 for the hard work.

I am looking for the WITH clause support in sqlite_orm so I can put subqueries aside and reuse them in my main SELECT statement. What I am trying to do is to calculate user retention of a user_activity table as the following:

WITH
register_user AS (
  SELECT
    uid, MIN(DATE(user_activity.timestamp)) register_date
  FROM user_activity
  GROUP BY uid
  HAVING register_date >= DATE('now', '-7 days')
),
register_user_count AS (
  SELECT
    R.register_date,
    COUNT(DISTINCT R.uid) AS user_count
  FROM register_user R
  GROUP BY R.register_date
)
SELECT
  R.register_date,
  CAST(julianday(DATE(A.timestamp)) AS INT) - CAST(julianday(R.register_date) AS INT) AS ndays,
  COUNT(DISTINCT A.uid) AS retention,
  C.user_count
FROM user_activity A
LEFT JOIN register_user R ON A.uid = R.uid
LEFT JOIN register_user_count C ON R.register_date = C.register_date
GROUP BY R.register_date, ndays
HAVING DATE(A.timestamp) >= DATE('now', '-7 days');

with the following sample output:

register_date  ndays       retention   user_count
-------------  ----------  ----------  ----------
2018-12-02     0           1           1
2018-12-02     6           1           1
2018-12-06     0           4           4
2018-12-06     5           3           4
2018-12-06     6           3           4
2018-12-10     0           2           2
2018-12-10     1           1           2
2018-12-10     2           1           2
2018-12-11     0           3           3
2018-12-11     1           1           3

I think queries of this complexity may not need to be constructed in the ORM way, raw SQL statements may be clearer and easier to understand in such cases.

@neevek neevek changed the title The "with" clause The WITH clause Dec 14, 2018
@fnc12
Copy link
Owner

fnc12 commented Dec 15, 2018

Hello. WITH is a great feature, I was thinking about it before. It is available to add it as an ORM API. I need to think how to perform it better. The main advantage of sqlite_orm is not to uses raw string queries but use some kind of compiled SQL in C++. So let me think. Thanks

@fnc12 fnc12 self-assigned this Dec 15, 2018
@fnc12
Copy link
Owner

fnc12 commented Dec 15, 2018

@neevek your example is perfect. I'd like to reproduce it with sqlite_orm in pure c++. But I need one thing from you - I need your schema and some data to reproduce it with sqlite3 shell first and with C++ later. Thanks in advance

@neevek
Copy link
Author

neevek commented Dec 16, 2018

@fnc12 Hello, the table user_activity is quite simple, it only has 3 columns, CREATE statements is as following:

CREAT TABLE user_activity (
id integer primary key,
uid integer,
timestamp datetime
);

The MIN datetime of multiple activity records for a uid is treated as the register time of that user, I am sorry I can't provide sample data because I am typing on a phone now, you can just insert a few activity records for each user with different timestamp.

@fnc12
Copy link
Owner

fnc12 commented Dec 17, 2018

first of all I need to add column alias feature. I suppose it to have a syntax like this:

//    SELECT id userId
//    FROM users
auto rows = storage.select(make_alias(&User::id, "userId"));

that returns the same as

//    SELECT id
//    FROM users
auto rows = storage.select(&User::id);

@fnc12
Copy link
Owner

fnc12 commented Dec 18, 2018

please check out column aliases in #226 in dev branch

@neevek
Copy link
Author

neevek commented Dec 19, 2018

It's great to see the progress update, but there's one thing, usage of custom alias, is too verbose to me, could it be possible to name and reference the alias with simple raw string, I am not sure if that would contradict with the design philosophy of the library, but that would make the query less verbose to some extent.

@fnc12
Copy link
Owner

fnc12 commented Dec 19, 2018

@neevek I've thought about it. I was able to make syntax like this:

auto rowsWithColumnAliases = storage.select(columns(as(&Employee::id, "EMP_ID"),
                                                    &Employee::name,
                                                    &Employee::age,
                                                    &Department::dept),
                                                  where(is_equal(alias("EMP_ID"), &Department::empId)));

but this options has string repeats which makes API vulnerable for literal errors. One of the main advantages of the lib is a static linkage with all query elements so you cannot make any literal misspelling errors.
Other options is not to use same literal but declare constant before query and use it:

auto myAlias = make_alias(&Employee::id, EMP_ID");
auto rowsWithColumnAliases = storage.select(columns(myAlias.as(),
                                                    &Employee::name,
                                                    &Employee::age,
                                                    &Department::dept),
                                                  where(is_equal(myAlias.get(), &Department::empId)));

This option is not bad but requires two statements for one query. So I decided to move every string to a dedicated class and users can be sure that there can not be any misspelling mistake.

Probably I forgot something and there is better way to perform aliases for sqlite_orm API. Please tell me that is your opinion in this case. Thanks

@neevek
Copy link
Author

neevek commented Dec 19, 2018

@fnc12 I am OK with option one. I am no professional on this issue, one thing I can think of to avoid spelling errors is to perform string literal comparison at compile time when referencing aliases, if an alias doesn't exist, referencing it yields compile time error.
For option two, I think it is not elegant for this task as you said, it requires two statements, also it is not intuitive that one has to call as() to name the alias and get() to reference it. Maybe a single overloaded alias function is enough, one with (const char *alias, member_fun_ptr), another with (const char *alias).

@fnc12
Copy link
Owner

fnc12 commented Dec 19, 2018

  1. One cannot compare string literals at compile time if literals are not declared as static explicitly. That is why I map every alias to a dedicated class. Of course compile time string comparison would be great. I miss it. But it is what it is(
  2. Overload is not an option cause both cases take template argument T. So what you give that will be taken. I think as and get ok cause in raw SQL statements are different: in the first case you write COLUMN as COLUMN_ALIAS, in the second you write COLUMN_ALIAS only

@fnc12
Copy link
Owner

fnc12 commented Dec 30, 2018

I'm trying to reproduce your initial query. Looks like you have an error here:

  SELECT
    uid, MIN(DATE(user_activity.timestamp)) register_date
  FROM user_activity
  GROUP BY uid
  HAVING register_date >= DATE('now', '-7 days')

user_activity.timestamp doesn't exist cause the schema your provided looks like

CREAT TABLE user_activity (
id integer primary key,
uid integer,
timestamp datetime
);

So I guess you wanted to say user_activity. datetime

@neevek
Copy link
Author

neevek commented Dec 30, 2018

What's the error? I cannot run SQL on phone now. I added a raw_query method to the library and used the SQL in my code, it run fine and had expected result.

-- edit
Oh sorry, I must have changed de coloum name when pasting the schema.

@fnc12
Copy link
Owner

fnc12 commented Dec 30, 2018

Anyway I'm still working on adding with feature one of main goals of sqlite_orm is that raw query is nor required at all

@neevek
Copy link
Author

neevek commented Dec 30, 2018

I totally understand that, but I needed to get the work done immediately, and that was the easiest and not so invasive way, that's why I didn't send pull request.

I am expecting the with clause, thanks for the hard work.

@fnc12
Copy link
Owner

fnc12 commented Dec 30, 2018

@neevek it's ok. This lib is totally open source and has open license so you're free to modify it as you want. First of all sqlite_orm must make working with SQLite easier for developers like you so it is ok.

@fnc12
Copy link
Owner

fnc12 commented Dec 31, 2018

I've added CAST feature #233

@fnc12
Copy link
Owner

fnc12 commented Dec 31, 2018

I've added julianday function in #234

@EricTheMagician
Copy link

@fnc12
Has it been implemented? I don't see it in the TODO, but I also don't find it in the source code.
So I assume that it's not a priority at the moment.

Anyways, I just wanted to pop in and say thanks for the great library!

In case you were wondering, I wanted to use the with clause for a recursive self-join.

Something like this:

WITH    q AS 
        (
        SELECT  *
        FROM   TASKS
        WHERE   PARENT_ID = 7
        UNION ALL
        SELECT  m.*
        FROM    TASKS m
        JOIN    q
        ON      m.PARENT_ID = q.ID
        )
SELECT  *
FROM    q
ORDER by  q.PARENT_ID

with a table like this:

CREATE TABLE 'TASKS' ( 'ID' INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL , 'DESCRIPTION' TEXT NOT NULL , 'STATUS' INTEGER NOT NULL , 'MODIFICATION' TEXT NOT NULL , PARENT_ID INTEGER)

@fnc12
Copy link
Owner

fnc12 commented Jan 18, 2022

@thejinx0r it has not been implemented. It doesn't exist in TODO list but it doesn't meant that it will not be implemented cause issue list is a TODO list #2.
Thanks for a helpful example. I'll try to implement WITH as fast as I can

firedaemon-cto pushed a commit to FireDaemon/sqlite_orm that referenced this issue Feb 7, 2023
* Shows yet another subquery hoisted into a CTE as required in issue fnc12#222
@trueqbit
Copy link
Collaborator

trueqbit commented Feb 8, 2023

@neevek @thejinx0r Perhaps you would like to try https://github.com/FireDaemon/sqlite_orm/tree/CTEs, for which there's also a draft PR.

There is an example there specifically for @neevek's query, along with a few others. I didn't have time to create good test data, but the query should work as expected.

@trueqbit trueqbit linked a pull request Feb 14, 2023 that will close this issue
@trueqbit trueqbit closed this as completed Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants