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

Set option for faster geography evaluation #7422

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

Conversation

scruti
Copy link
Collaborator

@scruti scruti commented Jan 16, 2025

Trello card URL

Changes in this PR:

By setting the use_spheroid = false flag in the ST_DWithin and ST_Distance PostGis over geography data, the operation benefits from a faster spherical calculation rather than the default computing on the spheroid determined by the SRID.

The downside is a slightly less accurate computation. But seems a good tradeoff since we're experiencing performance issues on the distance calculations and our distances are not that big to make a huge difference on the search results.

Copy link

Review app deployed to https://teaching-vacancies-review-pr-7422.test.teacherservices.cloud on AKS

@scruti scruti force-pushed the set-option-for-faster-geography-evaluation branch from 5d0bfc8 to d6dfb5a Compare January 17, 2025 09:28
By setting the "use_spheroid = false" flag in the ST_DWithin and
ST_Distance PostGis over geography data, the operation benefits by a
faster spherical calculation rather that the default computing on the
spheroid determined by the SRID.

The downside is a slightly less accurate computation. But seems a good
tradeoff since we're experiencing performance issues on the distance
calculations and our distances are not that big to make a huge
difference on the seach results.

Comment on why we repeat query for ordering

Ideally we would use the 'distance' alias on the ordering, but when the
location query gets combined with a 'pluck(:id)' (as it happens) call,
the generated SQL query ignores the first 'select ... AS distance' bit
of the query, and when 'distance' is called in the ordering, triggers an
exception.

For the time being, lets keep trusting the PostgreSQL query planner that
doesn't recalculate the repeated distance calculation (when already done
in the same query).
Remove obsolete brakeman ignored queries

Ignore false SQL injection brakeman warnings
@scruti scruti force-pushed the set-option-for-faster-geography-evaluation branch from d6dfb5a to 5f268fc Compare January 17, 2025 10:25
@@ -1,36 +1,48 @@
{
"ignored_warnings": [
{
"warning_type": "Cross-Site Scripting",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I had to add the modified SQL queries to the Brakeman ignore file, I also did an automated cleanup of legacy ignored warnings that are no longer needed. Hence all the code removal.

This is automated by brakeman -I tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant