NULLS FIRST | NULLS LAST

[SimpleForest]

Hello,

please let me know what you think of the general approach.

1. The static assert does not help. I can still compile ...nulls_first().nulls_first().
2. I implement it in core, not in postgresql speific module for the moment.

[SimpleForest]

I also have no clue, why here 3 commits are showing up. I am used to gitlab which handles git differently. I will clean up later.

[SimpleForest]

@MeanSquaredError Can you have a look here? I found finally some time to make a first attempt. You can see all in the third commit. The test is still messed up.

[MeanSquaredError]

Thanks for the PR, I will try to take a look at it shortly!

I also have no clue, why here 3 commits are showing up.

I think that you could try rebasing your branch against the latest main branch, e.g.

cd /path/to/your/copy/of/sqlpp
git remote add upstream https://github.com/rbock/sqlpp23.git
git pull --rebase upstream main
git push -f

This should synchronize the first two commits with the main branch and after the push they should disappear from this PR.

[MeanSquaredError]

OK, I rebased your branch on top of the latest main branch and then tried playing a bit with the nulls_first() and nulls_last() that you added.

I think it is OK to have the support for NULLS FIRST/NULLS LAST added to the core, since they are a part of the standard although their support is optional (according to Wikipedia they are described in SQL:2003 extension T611. Then the database connectors whose databases don't support it, could turn it off using some kind of type traits or a similar approach. I think right now PostgreSQL and SQLite3 support it and MySQL doesn't.

I wrote a basic build script + a very basic test program, which I will provide below in order to let everyone do the testing themselves.

build.sh
(you may need to adjust the paths in it)

#!/usr/bin/bash

g++ \
    -std=c++23 \
    -I/usr/local/projects/github/sqlpp23/include \
    -I/usr/local/projects/github/sqlpp23/tests/include/sqlpp23 \
    -Werror \
    -Wall \
    -Wextra \
    -Wpedantic \
mytest.cpp \
    -lpq

The actual test program, mytest.cpp

#include <tests/postgresql/make_test_connection.h>
#include <tests/postgresql/tables.h>

int main ()
{
	auto db = sqlpp::postgresql::make_test_connection ();
	auto tab = test::TabFoo {};
	db (
		select (tab.id)
		.from(tab)
//		.order_by (tab.id.asc ())
//		.order_by (tab.id.nulls_first ())
//		.order_by (tab.id.asc ().nulls_first ())
	);
	return 0;
}

The three commented-out lines correspond to three valid ORDER BY clauses:

ORDER BY id ASC
ORDER BY id NULLS FIRST
ORDER BY ASC NULLS FIRST

You can test each of them by removing the comment from the corresponding line in the source code. Although valid SQL, the first two compile but generate invalid SQL at runtime

This is the first (.order_by (tab.id.asc ())) at runtime:

connecting to the database server.
executing: 'SET TIME ZONE UTC;'
executing: 'SELECT tab_foo.id FROM tab_foo ORDER BY tab_foo.id ASCNULL'
terminate called after throwing an instance of 'sqlpp::postgresql::result_exception'
  what():  ERROR:  syntax error at or near "ASCNULL"
LINE 1: SELECT tab_foo.id FROM tab_foo ORDER BY tab_foo.id ASCNULL
                                                           ^

Aborted                    (core dumped) ./a.out

This is the second (.order_by (tab.id.nulls_first ())) at runtime:

connecting to the database server.
executing: 'SET TIME ZONE UTC;'
executing: 'SELECT tab_foo.id FROM tab_foo ORDER BY tab_foo.idNULL NULLS FIRST'
terminate called after throwing an instance of 'sqlpp::postgresql::result_exception'
  what():  ERROR:  column tab_foo.idnull does not exist
LINE 1: SELECT tab_foo.id FROM tab_foo ORDER BY tab_foo.idNULL NULLS...
                                                ^

Aborted                    (core dumped) ./a.out

The third one doesn't compile with g++ (GCC) 15.2.1 20260123 (Red Hat 15.2.1-7):

In file included from /usr/local/projects/github/sqlpp23/include/sqlpp23/core/query/dynamic.h:35,
                 from /usr/local/projects/github/sqlpp23/include/sqlpp23/core/basic/join_fwd.h:32,
                 from /usr/local/projects/github/sqlpp23/include/sqlpp23/core/basic/enable_join.h:30,
                 from /usr/local/projects/github/sqlpp23/include/sqlpp23/core/basic/table.h:32,
                 from /usr/local/projects/github/sqlpp23/include/sqlpp23/core/basic/schema_qualified_table.h:32,
                 from /usr/local/projects/github/sqlpp23/include/sqlpp23/sqlpp23.h:30,
                 from /usr/local/projects/github/sqlpp23/tests/include/sqlpp23/tests/postgresql/make_test_connection.h:37,
                 from mytest.cpp:1:
/usr/local/projects/github/sqlpp23/include/sqlpp23/core/operator/sort_order_expression.h: In instantiation of ‘constexpr sqlpp::sort_order_expression<L> sqlpp::sort_order_expression<L>::nulls_first() [with L = sqlpp::column_t<sqlpp::table_t<test::TabFoo_>, test::TabFoo_::Id>]’:
mytest.cpp:13:40:   required from here
   13 |                 .order_by (tab.id.asc ().nulls_first ())
      |                            ~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/usr/local/projects/github/sqlpp23/include/sqlpp23/core/operator/sort_order_expression.h:71:36: error: non-constant condition for static assertion
   71 |   static_assert(_rhs._rhs.has_value(), "nulls_first() can only be called when nulls_pos is not already set.");
      |                 ~~~~~~~~~~~~~~~~~~~^~
/usr/local/projects/github/sqlpp23/include/sqlpp23/core/operator/sort_order_expression.h:71:36:   in ‘constexpr’ expansion of ‘((sqlpp::sort_order_expression<sqlpp::column_t<sqlpp::table_t<test::TabFoo_>, test::TabFoo_::Id> >*)this)->sqlpp::sort_order_expression<sqlpp::column_t<sqlpp::table_t<test::TabFoo_>, test::TabFoo_::Id> >::_rhs.sqlpp::sort_order::_rhs.std::optional<sqlpp::nulls_pos>::has_value()’
/usr/include/c++/15/optional:1208:35:   in ‘constexpr’ expansion of ‘((const std::optional<sqlpp::nulls_pos>*)this)->std::optional<sqlpp::nulls_pos>::std::_Optional_base<sqlpp::nulls_pos, true, true>.std::_Optional_base<sqlpp::nulls_pos, true, true>::_M_is_engaged()’
/usr/local/projects/github/sqlpp23/include/sqlpp23/core/operator/sort_order_expression.h:71:17: error: ‘*(sqlpp::sort_order_expression<sqlpp::column_t<sqlpp::table_t<test::TabFoo_>, test::TabFoo_::Id> >*)this’ is not a constant expression
   71 |   static_assert(_rhs._rhs.has_value(), "nulls_first() can only be called when nulls_pos is not already set.");
      |                 ^~~~

I think that the main problem with the current implementation is that the type of the column order expression (sqlpp::sort_order_expression) does not encode any kind of information regarding the presence of direction (ASC or DESC) and nulls position (NULLS FIRST or NULLS LAST). Right now it just doesn't implement the asc() and desc() methods, which is fine if we support only direction (asc() or desc()), but if we want to support both direction and nulls position, then we need to somehow encode that in the type. So maybe sqlpp::sort_order_expression should have two additional boolean template parameters, e.g. HasDirection and HasNullsPosition, which we will use to decide if the methods asc(), desc(), nulls_first() and nulls_last() can be called on the corresponding sort expression.

The actual enabling/disabling of the methods can be done either through template specialization or through C++20 requires clauses attached to the corresponding methods. The approach with requires clauses seems easier to me, but I guess you need to try it to see whether it is simpler indeed.

Maybe @rbock could chime in and let us know if my proposed design seems OK to him, or maybe he can think of a better approach.

[rbock]

Thanks for the PR and the discussion so far.

I am a bit pressed for time, but quickly:

Disallowing nulls_first in the MySQL connector:
This is typically done in specializations of compatibility_check, see include/sqlpp23/mysql/constraints.h

asc/desc and nulls_first/last:
Currently, order_by expressions require asc or desc. I want to keep it this way. That means that .order_by(t.id.nulls_first()) would be invalid.

And then it is rather straight forward:

• Calling .asc() or .desc() on an expression turns it into a sort_order_expression which offers .nulls_first() and .nulls_last(). Calling those would return a new type (with no .nulls_first/last).

Hope this makes sense?

Cheers,
Roland

[MeanSquaredError]

@rbock
Thanks for the clarification. Keeping the ordering direction (ASC/DESC) mandatory does simplify things indeed at the cost of being slightly more restrictive than the actual SQL standard.