Skip to content

Revert "Add histograms for optimizer cost calculation"#22661

Merged
feilong-liu merged 4 commits intoprestodb:masterfrom
feilong-liu:fix_timeout
May 3, 2024
Merged

Revert "Add histograms for optimizer cost calculation"#22661
feilong-liu merged 4 commits intoprestodb:masterfrom
feilong-liu:fix_timeout

Conversation

@feilong-liu
Copy link
Contributor

@feilong-liu feilong-liu commented May 3, 2024

This reverts commit e8c4659. This reverts PR #21236

As there are other PRs referring to the changes in this PR, hence also reverting #22327 and #22395 here

Description

This reverted commit will make query with large IN clause to be super slow. In the test query I ran, the latency increase from 7 seconds to 4 and half minutes!

If not reverting, it will take 4 and half minutes to execute this query.

presto:tpch> select orderpriority, sum(totalprice) from lineitem join orders on lineitem.orderkey = orders.orderkey where orders.orderkey in ( 0 , 1 , 2 , 3 , 4 , 5 , 6 , 7 , 8 , 9 , ..., 9995 , 9996 , 9997 , 9998 , 9999 ) group by 1;  -- the IN clause has element from 0 to 9999
  orderpriority  |        _col1         NG] i[0 0B 0B] o[0 0B 0B] splits[65/29/0]B] splits[0/0/0]
-----------------+----------------------
 2-HIGH          | 3.3029310959000003E8 
 5-LOW           | 3.4520414947999984E8 
 3-MEDIUM        | 3.5743611102999985E8 
 1-URGENT        | 3.6284532178999996E8 
 4-NOT SPECIFIED |       3.6984263411E8 
(5 rows)

Query 20240503_165715_00066_i9y9u, FINISHED, 4 nodes
http://localhost:8080/ui/query.html?20240503_165715_00066_i9y9u
Splits: 94 total, 94 done (100.00%)
[Latency: client-side: 4:30, server-side: 4:30] [53.7K rows, 1.74MB] [199 rows/s, 6.6KB/s]

After revert the change, it will take only 7 seconds!

presto:tpch> select orderpriority, sum(totalprice) from lineitem join orders on lineitem.orderkey = orders.orderkey where orders.orderkey in ( 0 , 1 , 2 , 3 , 4 , 5 , 6 , 7 , 8 , 9 , ..., 9995 , 9996 , 9997 , 9998 , 9999 ) group by 1;  -- the IN clause has element from 0 to 9999
  orderpriority  |        _col1         NG_FOR_PREREQUISITES] i[0 0B 0B] o[0 0B 0B] splits[0/0/0]
-----------------+----------------------
 4-NOT SPECIFIED |  3.698426341100003E8 
 5-LOW           | 3.4520414947999984E8 
 2-HIGH          |  3.302931095899997E8 
 1-URGENT        | 3.6284532179000014E8 
 3-MEDIUM        |  3.574361110299997E8 
(5 rows)

Query 20240503_165241_00070_jnx6t, FINISHED, 4 nodes
http://localhost:8080/ui/query.html?20240503_165241_00070_jnx6t
Splits: 95 total, 95 done (100.00%)
[Latency: client-side: 0:07, server-side: 0:07] [64.2K rows, 1.74MB] [9.12K rows/s, 253KB/s]

Motivation and Context

The above is a synthetic query I created to demonstrate the problem.
We already observe queries in our production seeing longer latency and even fail/timeout.

Impact

Fix latency regression and query timeout failure

Test Plan

Existing unit tests, and also tested locally end to end with production queries.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Fix the latency regression for queries with large IN clause

kaikalur
kaikalur previously approved these changes May 3, 2024
@github-actions
Copy link

github-actions bot commented May 3, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff 2381d19...a07d5a4.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/connector/iceberg.rst

@ZacBlanco
Copy link
Contributor

Thanks for identifying this. I found the root cause. I'll see to fixing it while this is reverted.

rschlussel
rschlussel previously approved these changes May 3, 2024
Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

thanks for giving a detailed explanation of the reason for reverting!

hantangwangd
hantangwangd previously approved these changes May 3, 2024
Fix this test as part of revert PR prestodb#22661
I chose to fix forward because the original PR is mainy for flaky test fix and has
light dependency on the reverted PR hence fix forward is a better option.
@feilong-liu feilong-liu dismissed stale reviews from hantangwangd and rschlussel via a07d5a4 May 3, 2024 18:16
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)
Pull branch, local build of docs, looks good.

@feilong-liu feilong-liu merged commit a9790c3 into prestodb:master May 3, 2024
@feilong-liu feilong-liu deleted the fix_timeout branch May 3, 2024 20:37
zacw7 pushed a commit that referenced this pull request May 7, 2024
Fix this test as part of revert PR #22661
I chose to fix forward because the original PR is mainy for flaky test fix and has
light dependency on the reverted PR hence fix forward is a better option.
@wanglinsong wanglinsong mentioned this pull request Jun 25, 2024
36 tasks
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.

7 participants