Skip to content

NULLS FIRST | NULLS LAST #102

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.

Sign up for GitHub

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

Draft
SimpleForest wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

NULLS FIRST | NULLS LAST #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions include/sqlpp23/core/operator/comparison_functions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,19 +169,42 @@ enum class sort_type {
template <typename L>
requires(values_are_comparable<L, L>::value)
constexpr auto asc(L l) -> sort_order_expression<L> {
return {l, sort_type::asc};
return {std::move(l), sort_type::asc};
Copy link
Owner

@rbock rbock Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you undo the other changes in this file, these would stand out as unrelated refactoring.

These are good, but should go into a separate PR.

}

template <typename L>
requires(values_are_comparable<L, L>::value)
constexpr auto desc(L l) -> sort_order_expression<L> {
return {l, sort_type::desc};
return {std::move(l), sort_type::desc};
}

template <typename L>
requires(values_are_comparable<L, L>::value)
constexpr auto order(L l, sort_type order) -> sort_order_expression<L> {
return {l, order};
return {std::move(l), order};
}

enum class nulls_pos {
Copy link
Owner

@rbock rbock Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go to sort_order_expression.h now

first,
last,
};

template <typename L>
Copy link
Owner

@rbock rbock Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be dropped given the changes to sort_order_expression.

requires(values_are_comparable<L, L>::value)
constexpr auto nulls_first(L l) -> sort_order_expression<L> {
return {std::move(l), {std::nullopt , nulls_pos::first}};
}

template <typename L>
requires(values_are_comparable<L, L>::value)
constexpr auto nulls_last(L l) -> sort_order_expression<L> {
return {std::move(l), {std::nullopt , nulls_pos::last}};
}

template <typename L>
requires(values_are_comparable<L, L>::value)
constexpr auto nulls_order(L l, nulls_pos pos) -> sort_order_expression<L> {
return {std::move(l), {std::nullopt , std::move(pos)}};
}

} // namespace sqlpp
18 changes: 18 additions & 0 deletions include/sqlpp23/core/operator/enable_comparison.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,24 @@ class enable_comparison {
return ::sqlpp::order(std::forward<Expr>(self), t);
}

template <typename Expr>
Copy link
Owner

@rbock rbock Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would go away, right? nulls_first and friends would only make sense in an order_expression.

constexpr auto nulls_first(this Expr&& self)
-> decltype(::sqlpp::nulls_first(std::forward<Expr>(self))) {
return ::sqlpp::nulls_first(std::forward<Expr>(self));
}

template <typename Expr>
constexpr auto nulls_last(this Expr&& self)
-> decltype(::sqlpp::nulls_last(std::forward<Expr>(self))) {
return ::sqlpp::nulls_last(std::forward<Expr>(self));
}

template <typename Expr>
constexpr auto nulls_order(this Expr&& self, ::sqlpp::nulls_pos t)
-> decltype(::sqlpp::nulls_order(std::forward<Expr>(self), t)) {
return ::sqlpp::nulls_order(std::forward<Expr>(self), t);
}

template <typename Expr, typename R>
constexpr auto like(this Expr&& self, R r)
-> decltype(::sqlpp::like(std::forward<Expr>(self), std::move(r))) {
Expand Down
56 changes: 54 additions & 2 deletions include/sqlpp23/core/operator/sort_order_expression.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,60 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <sqlpp23/core/reader.h>

namespace sqlpp {

template <typename L>
struct sort_order_with_nulls_expression {
constexpr sort_order_with_nulls_expression(sort_order_expression<L> lhs, nulls_pos rhs)
: _lhs(std::move(lhs)), _rhs(std::move(rhs)) {}
sort_order_with_nulls_expression(const sort_order_with_nulls_expression&) = default;
sort_order_with_nulls_expression(sort_order_with_nulls_expression&&) = default;
sort_order_with_nulls_expression& operator=(const sort_order_with_nulls_expression&) = default;
sort_order_with_nulls_expression& operator=(sort_order_with_nulls_expression&&) = default;
~sort_order_with_nulls_expression() = default;

private:
friend reader_t;
const sort_order_expression<L> _lhs;
Copy link
Owner

@rbock rbock Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use const for the members.

These are private data members. Making them const does make the API any better.
But it could hurt if someone wanted to use copy or move assignment.

Also (general recommendation), it pays off to follow patterns of the existing code, e.g. for readability.

It might be perfectly fine to change the coding style, to introduce new patterns, etc, but that should happen in separate refactoring CLs.

const nulls_pos _rhs;
};

template <typename L>
struct sort_order_expression {
constexpr sort_order_expression(L l, sort_type r)
Copy link
Contributor

@MeanSquaredError MeanSquaredError Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to keep the old parameter names (l and r) and not rename them unless it is really required. This will decrease the PR size and will simplify the PR review.

: _lhs(std::move(l)), _rhs(std::move(r)) {}
constexpr sort_order_expression(L lhs, sort_type rhs)
: _lhs(std::move(lhs)), _rhs(std::move(rhs)) {}
sort_order_expression(const sort_order_expression&) = default;
sort_order_expression(sort_order_expression&&) = default;
sort_order_expression& operator=(const sort_order_expression&) = default;
sort_order_expression& operator=(sort_order_expression&&) = default;
~sort_order_expression() = default;

constexpr auto nulls_first() -> sort_order_with_nulls_expression<L>;
Copy link
Owner

@rbock rbock Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please stay aligned with the library style (functions are defined inline).

constexpr auto nulls_last() -> sort_order_with_nulls_expression<L>;
constexpr auto nulls_order(::sqlpp::nulls_pos t) -> sort_order_with_nulls_expression<L>;

private:
friend reader_t;
L _lhs;
sort_type _rhs;
};



template <typename L>
constexpr auto sort_order_expression<L>::nulls_first() -> sort_order_with_nulls_expression<L> {
return {std::move(_lhs), ::sqlpp::nulls_pos::first};
}

template <typename L>
constexpr auto sort_order_expression<L>::nulls_last() -> sort_order_with_nulls_expression<L> {
return {std::move(_lhs), ::sqlpp::nulls_pos::last};
};

template <typename L>
constexpr auto sort_order_expression<L>::nulls_order(::sqlpp::nulls_pos t) -> sort_order_with_nulls_expression<L> {
return {std::move(_lhs), std::move(t)};
}

template <typename L>
struct nodes_of<sort_order_expression<L>> {
using type = detail::type_vector<L>;
Expand All @@ -64,6 +102,20 @@ auto to_sql_string(Context&, const sort_type& t) -> std::string {
return " DESC";
}

template <typename Context>
auto to_sql_string(Context&, const nulls_pos& t) -> std::string {
if (t == nulls_pos::first) {
return " NULLS FIRST";
}
return " NULLS LAST";
}

template <typename Context>
auto to_sql_string(Context& context, const sort_order& t)
-> std::string {
return to_sql_string(context, read.lhs(t)) + to_sql_string(context, read.rhs(t));
}

template <typename Context, typename L>
auto to_sql_string(Context& context, const sort_order_expression<L>& t)
-> std::string {
Expand Down