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

Remove redundant S2 cells queries for operational intents #1042

Conversation

BenjaminPelletier
Copy link
Member

For what I believe are historical reasons (entity cells used to be in a separate table from the entities themselves rather than in a column of the entity), the S2 cells for an operational intent are currently retrieved separately and explicitly for each individual operational intent even though the cells had already been retrieved in the initial query for operational intent information. This resulted in an enormous number of additional queries -- one for each operational intent result -- which would cause, e.g., an operational intent search to perform 200+ SQL queries if the search returned 200 operational intents. All of these additional queries are unnecessary because each entity's S2 cells are already retrieved on the initial query.

This PR fixes that problem by simply deleting the redundant separate population of operational intent cells. Thanks to @BradNicolle for the catch and suggestion.

@BenjaminPelletier
Copy link
Member Author

Note: the content being deleted cases int64 cell IDs to uint64 before casting to s2.CellIDs whereas OperationalIntent.SetCells casts the int64s directly to s2.CellIDs. We should verify that the intermediate uint64 cast is not necessary.

@Shastick
Copy link
Contributor

Shastick commented Jun 9, 2024

I ran the qualifier against a locally built version of this PR and everything passed.

(I still haven't gotten to running the qualifier as part of the CI as @barroco suggested , I'll create an issue and put it on my stack)

Considering that the code does not seem to be required, the PR looks good to me.

@barroco
Copy link
Contributor

barroco commented Jun 10, 2024

Note: the content being deleted cases int64 cell IDs to uint64 before casting to s2.CellIDs whereas OperationalIntent.SetCells casts the int64s directly to s2.CellIDs. We should verify that the intermediate uint64 cast is not necessary.

s2.CellID type is an alias of uint64, so I guess the cast was transparent.

@BenjaminPelletier BenjaminPelletier merged commit ce9e97d into interuss:master Jun 10, 2024
6 checks passed
@BenjaminPelletier BenjaminPelletier deleted the remove-redundant-cells-queries branch June 10, 2024 15:25
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.

3 participants