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

[nexus] Experiment with using closer-to-raw-SQL pagination #7717

Draft
wants to merge 209 commits into
base: main
Choose a base branch
from

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Mar 4, 2025

The pagination functions - in particular, the paginated_multicolumn function - is notorious
for having incredibly complex generic parameters.

For context, see:

pub fn paginated_multicolumn<T, C1, C2, M1, M2>(
query: T,
(c1, c2): (C1, C2),
pagparams: &DataPageParams<'_, (M1, M2)>,
) -> <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output
where
// T is a table^H^H^H^H^Hquery source which can create a BoxedQuery.
T: QuerySource,
T: AsQuery,
<T as QuerySource>::DefaultSelection:
Expression<SqlType = <T as AsQuery>::SqlType>,
T::Query: query_methods::BoxedDsl<'static, Pg>,
// Required for...everything.
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output: QueryDsl,
// C1 & C2 are columns which appear in T.
C1: 'static + Column + Copy + ExpressionMethods,
C2: 'static + Column + Copy + ExpressionMethods,
// Required to compare the columns with the marker types.
C1::SqlType: SqlType,
C2::SqlType: SqlType,
M1: Clone + AsExpression<C1::SqlType>,
M2: Clone + AsExpression<C2::SqlType>,
// Necessary for `query.limit(...)`
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::LimitDsl<
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// Necessary for "query.order(c1.desc())"
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::OrderDsl<
Desc<C1>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// Necessary for "query.order(...).then_order_by(c2.desc())"
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::ThenOrderDsl<
Desc<C2>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// Necessary for "query.order(c1.asc())"
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::OrderDsl<
Asc<C1>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// Necessary for "query.order(...).then_order_by(c2.asc())"
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::ThenOrderDsl<
Asc<C2>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// We'd like to be able to call:
//
// c1.eq(v1).and(c2.gt(v2))
//
// This means "c1.eq(v1)" must implement BoolExpressionMethods, and
// satisfy the requirements of the ".and" method.
//
// The LHS (c1.eq(v1)) must be a boolean expression:
Eq<C1, M1>: Expression<SqlType = Bool>,
// The RHS (c2.gt(v2)) must be a boolean expression:
Gt<C2, M2>: Expression<SqlType = Bool>,
// Putting it together, we should be able to filter by LHS.and(RHS):
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::FilterDsl<
And<Eq<C1, M1>, Gt<C2, M2>>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// We'd also like to be able to call:
//
// c1.eq(v1).and(c2.lt(v2))
//
// We've already defined the bound on the LHS, so we add the equivalent
// bounds on the RHS for the "Less than" variant.
Lt<C2, M2>: Expression<SqlType = Bool>,
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::FilterDsl<
And<Eq<C1, M1>, Lt<C2, M2>>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// Necessary for "query.or_filter(c1.gt(v1))"
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::OrFilterDsl<
Gt<C1, M1>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
// Necessary for "query.or_filter(c1.lt(v1))"
<T::Query as query_methods::BoxedDsl<'static, Pg>>::Output:
query_methods::OrFilterDsl<
Lt<C1, M1>,
Output = <T::Query as query_methods::BoxedDsl<'static, Pg>>::Output,
>,
{

To reduce this pain, this PR provides pagination helpers which side-step Diesel's type-checking,
aiming to provide a more flexible option for pagination which can work across arbitrary JOINs,
UNIONs, etc, without relying on compile-time type-checking.

fmt
@@ -88,14 +88,14 @@ type BoxedQuery = diesel::query_builder::BoxedSqlQuery<
//
// But this relies on nightly features.
pub struct QueryBuilder {
query: BoxedQuery,
query: Option<BoxedQuery>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To make the RawPaginator API with the interface that exists, these functions needed to act on &mut self instead of self.

This was a breaking change, and it requires using an Option<BoxQuery> internally, but that inner type detail should not be visible to callers.

}

impl<M1, M2> RawPaginatorWithParams<'_, (M1, M2)> {
pub fn paginate_by_columns<BindSt1, BindSt2>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems way for feasible to make this generic across a tuple of an arbitrary length -- the generic parameters are much simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I picked the affinity "list" APIs because those couldn't use the old pagination functions anyway - they had to be manual, to paginate over a UNION subquery.

I've converted everything in this file to use the RawPaginator.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file also uses a combination of paginated and paginated_multicolumn.

I picked it as a case where "there are a lot of joins + filters", to see if the new helpers can work.

smklein added 8 commits March 3, 2025 17:57
Base automatically changed from affinity-expose-name to main March 24, 2025 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant