Skip to content

Fixed index performance regression#1363

Merged
Hydrocharged merged 2 commits intomainfrom
daylon/index-regression
Apr 9, 2025
Merged

Fixed index performance regression#1363
Hydrocharged merged 2 commits intomainfrom
daylon/index-regression

Conversation

@Hydrocharged
Copy link
Copy Markdown
Collaborator

@Hydrocharged Hydrocharged commented Apr 8, 2025

I've left a fairly detailed comment for the future, but it doesn't quite explain the exact issue here. It's more so to prevent making a change that would lead to the issue again.

In a nutshell, we were always adding boolean literal expressions in our start and stop expressions, which define the start and stop positions for the underlying tuple iterator. With the Dolt change to fix a case of incorrect results, we were met with even worse results in some circumstances. With the original logic, these literals were harmless to include in multi-expression ranges, but they fundamentally changed the behavior of the stop position with the new logic. They're required to be there though if there are no other expressions, hence it made sense to just always include them. The behavioral change isn't obvious at first glance, hence the need for the long comment (which should make it appear relatively obvious after reading).

This also makes use of QuickFunction where possible, which should always be true for tuple-level filtering, and should provide a small speedup in those cases.

@Hydrocharged Hydrocharged requested a review from zachmu April 8, 2025 12:29
@Hydrocharged Hydrocharged force-pushed the daylon/index-regression branch from ff39d1f to 145abc0 Compare April 8, 2025 12:38
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2025

Main PR
covering_index_scan_postgres 343.59/s 348.63/s +1.4%
index_join_postgres 153.39/s 153.66/s +0.1%
index_join_scan_postgres 184.11/s 185.15/s +0.5%
index_scan_postgres 12.57/s 12.93/s +2.8%
oltp_point_select 2665.37/s 2677.55/s +0.4%
oltp_read_only 715.40/s ${\color{lightgreen}1860.40/s}$ ${\color{lightgreen}+160.0\%}$
select_random_points 114.97/s 115.10/s +0.1%
select_random_ranges 8.55/s ${\color{lightgreen}133.23/s}$ ${\color{lightgreen}+1458.2\%}$
table_scan_postgres 12.00/s 12.28/s +2.3%
types_table_scan_postgres 5.60/s 5.66/s +1.0%

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2025

Main PR
Total 42090 42090
Successful 15578 15578
Failures 26512 26512
Partial Successes1 5208 5208
Main PR
Successful 37.0112% 37.0112%
Failures 62.9888% 62.9888%

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

Copy link
Copy Markdown
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM, just the one comment

@Hydrocharged Hydrocharged enabled auto-merge April 9, 2025 08:26
@Hydrocharged Hydrocharged merged commit 314abbb into main Apr 9, 2025
14 checks passed
@Hydrocharged Hydrocharged deleted the daylon/index-regression branch April 9, 2025 08:53
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