Skip to content

Include already collected stats in QueryCompletionEvent#19796

Merged
sopel39 merged 5 commits intotrinodb:masterfrom
starburstdata:ks/always_collect_stats
Nov 21, 2023
Merged

Include already collected stats in QueryCompletionEvent#19796
sopel39 merged 5 commits intotrinodb:masterfrom
starburstdata:ks/always_collect_stats

Conversation

@sopel39
Copy link
Copy Markdown
Member

@sopel39 sopel39 commented Nov 17, 2023 

If stats collection was not requested explicitly, then use statistics
that were fetched and cached during planning.

@cla-bot cla-bot bot added the cla-signed label Nov 17, 2023
@sopel39 sopel39 removed the request for review from gaurav8297 November 17, 2023 13:11
@sopel39 sopel39 force-pushed the ks/always_collect_stats branch from a5a8299 to 1ff53e6 Compare November 17, 2023 13:12
@findepi
Copy link
Copy Markdown
Member

findepi commented Nov 17, 2023

Always collect statistics at the end of planning

i am not sure i understand what the change is doing.
we definitely do not want to alway collect statistics, this may be expensive.

is this change about EXPLAIN, or query completion event, or something else?

@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Nov 17, 2023

we definitely do not want to alway collect statistics, this may be expensive.

It's not always collecting statistics, but used already collected per commit description

is this change about EXPLAIN, or query completion event, or something else?

Yes, we want EXPLAIN with stats in query completion event.

raunaqmorarka
raunaqmorarka reviewed Nov 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

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

lgtm % comment

@trinodb trinodb deleted a comment from lukasz-stec Nov 20, 2023
@sopel39 sopel39 force-pushed the ks/always_collect_stats branch from 1ff53e6 to 8f57d5f Compare November 20, 2023 16:15
sopel39
sopel39 commented Nov 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member Author

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

ac

@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Nov 20, 2023

ptal again @raunaqmorarka @findepi

@sopel39
Prevent ValuesStatsRule from causing planning failures
4fe7645 
ValuesStatsRule is calling evaluateConstantExpression
which might fail for certain expressions like division by 0.
@sopel39 sopel39 force-pushed the ks/always_collect_stats branch from 8f57d5f to 49575eb Compare November 20, 2023 22:15
@github-actions github-actions bot added tests:hive hive Hive connector labels Nov 20, 2023
@sopel39 sopel39 force-pushed the ks/always_collect_stats branch from 49575eb to 1975a58 Compare November 20, 2023 22:34
@sopel39
Remove try catch around DeterminePartitionCount stats estimation
48db427 
It's no longer needed as stats rules have been improved
@sopel39
Always collect statistics at the end of planning
22fcbbc 
If stats collection was not requested explicitly, then use statistics
that were fetched and cached during planning.
@sopel39 sopel39 force-pushed the ks/always_collect_stats branch from 1975a58 to 22fcbbc Compare November 21, 2023 08:25
@findepi
Copy link
Copy Markdown
Member

findepi commented Nov 21, 2023

Always collect statistics at the end of planning

Would it be fair to call this PR "Include already collected stats in QueryCompletionEvent" ?

@sopel39 sopel39 changed the title Always collect statistics at the end of planning Include already collected stats in QueryCompletionEvent Nov 21, 2023
@sopel39 sopel39 merged commit 564bf72 into trinodb:master Nov 21, 2023
@sopel39 sopel39 deleted the ks/always_collect_stats branch November 21, 2023 10:27
@github-actions github-actions bot added this to the 434 milestone Nov 21, 2023
@mosabua
Copy link
Copy Markdown
Member

mosabua commented Nov 21, 2023

I assume no RN entry needed @sopel39 and @findepi ...

@mosabua mosabua mentioned this pull request Nov 21, 2023
Merged
@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Nov 22, 2023 •
edited
Loading

@mosabua we can skip release notes IMO, I think it's too low level and opportunistic (so stats are not guaranteed to be there)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raunaqmorarka raunaqmorarka raunaqmorarka approved these changes

@lukasz-stec lukasz-stec Awaiting requested review from lukasz-stec

@findepi findepi Awaiting requested review from findepi

Assignees

No one assigned

Labels

cla-signed hive Hive connector

Milestone

434

Development

Successfully merging this pull request may close these issues.

4 participants

@sopel39 @findepi @mosabua @raunaqmorarka