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

optimize mixed str/int content #127

Merged
merged 3 commits into from
May 4, 2024

Conversation

igorkramaric
Copy link
Contributor

@igorkramaric igorkramaric commented May 3, 2024

In addition to #126 this PR takes into account also integers and treats them same way as strings when it comes to optimization.
Normally, the "integers only" optimization would be applied as well, but it is preempted by the earlier existence optimization, preventing it from being triggered.

Code excerpt for the record:

        if not breaks_existence_optimizer(children):
            return self.optimize_existence(children, sql_expr, "?&", "AND")

        if not breaks_equality_optimizer(children):
            return self.optimize_equality_and(children, sql_expr)

(inverted optimization ruin performance, should be avoided. Existence should be the first)

Motivation

Vix: It turns out that this change speeds up the calculation by further 31% compared to the first (strings only) optimization step.
All Non-Vix routers: from 321.8 seconds down to 239.2 seconds, which is 25% (relative to string only optimization) or 264.2 seconds down to 239.2 seconds which is only 9.5% faster.

Conclusion this PR fixes the performance for non-Vix LFRs which got ruined by the first optimization.

Overall

Vix: 62% faster
Slowest vix: 27x faster
Non-vix: 9.5% faster

@igorkramaric
Copy link
Contributor Author

Extra:
Discovered a bug. In a "multiple string/integer equality AND expression" when using the same key twice, with different values the count of results should obviously be zero. However, optimized SQL may easily give result other than zero.

Optimization should not be skipped whenever there are any non-unique keys.

@igorkramaric igorkramaric merged commit 525e31e into mediapredict:master May 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant