Skip to content

Conversation

helloiamlukas
Copy link
Contributor

fixes #153

@what-the-diff
Copy link

what-the-diff bot commented May 17, 2023

PR Summary

  • New method added to EntryQueryBuilder class
    A new function has been introduced to enhance the class, improving its functionality.
  • Improved orderBy() method for proper sorting
    The orderBy() method now correctly sorts integer or float fields using raw SQL, fixing the issue with Eloquent ORM methods not working for these data types.

@ryanmitchell
Copy link
Contributor

This solution is MySQL specific, so that would mean it's not something we could incorporate into a release. Im very open to finding an alternative way to sort this, especially if we can apply it to other column types too.

By the way, as a work around, you could make a migration to create a stored/generated field in MySQL with the value of the column and the data type you want. MySQL is smart enough to use that column for performance reasons even when you use data->xxx.

@helloiamlukas
Copy link
Contributor Author

What do you mean by this solution being MySQL specific? This solutions applies to PostgreSQL and SQLite too, or am I missing something?
We use this fix for PostgreSQL in production for a while now without any problems.

@ryanmitchell
Copy link
Contributor

ryanmitchell commented Jun 14, 2023

Sorry, wrong way round. I should have said it doesn't work in MySQL.

In MySQL the syntax would need to be orderByRaw("data->\$.'{$column}' {$direction}") otherwise you get an "invalid JSON path expression" error.

SQLite seems to use the same syntax: https://www.sqlite.org/json1.html#jptr

Am I missing something?

@helloiamlukas
Copy link
Contributor Author

Oh, I see the problem now. And I am with you that there should probably no MySQL/Postgres/SQLite specific code included in the query builder.

One solution that came across my mind: We could extend the grammars and add a method compileFloatValueCast. This could be used in the query builder then. I'm not sure if this is the right approach though.

@ryanmitchell
Copy link
Contributor

It's a hard one to solve while not making it specific to a certain database engine.
My current thinking is we need to CAST the value as a column type (I think this is generic to all 3 engines).
Your use case is numeric, but the same issue exists for dates so it needs to be solved in a non-specific way.

@ryanmitchell
Copy link
Contributor

@helloiamlukas ok, heres a change of approach to use CAST(x as y) where available.
SQLite messes things up a little bit so I've hard coded a change for it on dates, which isnt ideal, but we can revisit that if it causes issues down the line.

Let me know your thoughts on this.

@helloiamlukas
Copy link
Contributor Author

Looking good to me, thank you :)

One more thing we should maybe consider: Text fieldtypes can have a number or date input. Should we also cast text fieldtypes when the input_type is number or date?

@ryanmitchell
Copy link
Contributor

I think I'm happy enough (for now) to say that you have to use one of the explicit field types... we can always revisit that later down the road if that needs to change.

Thanks for your help!

@ryanmitchell ryanmitchell changed the title Prevent string conversion of numeric fields when using orderBy Ensure it is possible to orderBy any integer, float or date fields Jun 19, 2023
@ryanmitchell ryanmitchell changed the title Ensure it is possible to orderBy any integer, float or date fields Ensure it is possible to order entries any integer, float or date fields Jun 26, 2023
@ryanmitchell ryanmitchell changed the title Ensure it is possible to order entries any integer, float or date fields Ensure it is possible to order entries by integer, float or date fields Jun 26, 2023
@ryanmitchell ryanmitchell merged commit fc0819d into statamic:master Jun 28, 2023
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.

Sorting does not work for numeric fields

2 participants