Skip to content

Order by kth column #966

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

Merged
merged 29 commits into from
Mar 31, 2022
Merged

Order by kth column #966

merged 29 commits into from
Mar 31, 2022

Conversation

trueqbit
Copy link
Collaborator

Addresses #950.

Copy link
Owner

@fnc12 fnc12 left a comment

Choose a reason for hiding this comment

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

I am not sure that 1_nth_col must be added because when user wants to write ORDER BY 1 using sqlite_orm then he/she will write order_by(1) by default. It is not clear for users why order_by(1) will not work but order_by(1_nth_col) will so it would be better to support order_by(1) instead

@firedaemon-cto firedaemon-cto force-pushed the issues/order_by_kth_col branch from 38454bf to c96d19e Compare March 22, 2022 21:15
@firedaemon-cto firedaemon-cto force-pushed the issues/order_by_kth_col branch from ac35db0 to 1d4ace0 Compare March 22, 2022 21:19
@trueqbit trueqbit requested a review from fnc12 March 23, 2022 08:47
@fnc12
Copy link
Owner

fnc12 commented Mar 23, 2022

I'll review once #977 is closed

Copy link
Owner

@fnc12 fnc12 left a comment

Choose a reason for hiding this comment

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

Let's leave API like order_by(2). Let's add an overload for order_by for bindables or types associated with integer_printer but let's leave API like this cause in SQL API is the same for both

  1. ORDER BY 2
  2. ORDER BY CASE ... END

// FROM marvel
// ORDER BY 2
auto rows = storage.select(columns(&MarvelHero::name, as<colalias_i>(instr(&MarvelHero::abilities, "o"))),
order_by(get<colalias_2>()));
Copy link
Owner

Choose a reason for hiding this comment

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

order_by(2) is more intuitive and available to implement. get<colalias_2>() looks weird for newbies and raises questions

@trueqbit
Copy link
Collaborator Author

Alright, there's now an overload for order_by() for types associated with integer_printer.

struct literal_holder {
using type = T;

T value;
Copy link
Owner

Choose a reason for hiding this comment

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

better replace T with type here

@fnc12 fnc12 merged commit 5e48435 into fnc12:dev Mar 31, 2022
@fnc12
Copy link
Owner

fnc12 commented Mar 31, 2022

thanks @trueqbit

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.

2 participants