Skip to content

Document table statistics property for Hive connector#10525

Merged
electrum merged 1 commit intotrinodb:masterfrom
rosewms:rw-hive-stats
Jan 14, 2022
Merged

Document table statistics property for Hive connector#10525
electrum merged 1 commit intotrinodb:masterfrom
rosewms:rw-hive-stats

Conversation

@rosewms
Copy link
Copy Markdown
Contributor

@rosewms rosewms commented Jan 10, 2022

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jan 10, 2022
mosabua
mosabua requested changes Jan 10, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Break up into separate PRs please as well

Comment thread docs/src/main/sphinx/connector/hive.rst Outdated
Comment thread docs/src/main/sphinx/connector/hive.rst Outdated
m57lyra
m57lyra reviewed Jan 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@m57lyra m57lyra left a comment

Choose a reason for hiding this comment

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

This needs sorting out

Comment thread docs/src/main/sphinx/connector/hive.rst Outdated
mosabua
mosabua approved these changes Jan 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Please update the title of the PR to match the commit message as discussed.

Also confirm that this builds locally to verify that the GHA failure is a false alarm.

@rosewms rosewms requested a review from m57lyra January 12, 2022 19:26
@rosewms rosewms changed the title Add hive statistics properties Document table statistics property for Hive connector Jan 12, 2022
@rosewms rosewms requested a review from raunaqmorarka January 12, 2022 19:27
m57lyra
m57lyra reviewed Jan 13, 2022
View reviewed changes
Comment thread docs/src/main/sphinx/connector/hive.rst Outdated
Copy link
Copy Markdown
Contributor

@m57lyra m57lyra Jan 13, 2022

Choose a reason for hiding this comment

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

it is confusing that the name of this is *enabled, but the description starts out with "Disable..." but the default is true.

hashhar
hashhar reviewed Jan 13, 2022
View reviewed changes
Comment thread docs/src/main/sphinx/connector/hive.rst Outdated
@rosewms rosewms requested review from hashhar and m57lyra January 13, 2022 21:36
hashhar
hashhar approved these changes Jan 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Looks good % nit.

Comment thread docs/src/main/sphinx/connector/hive.rst Outdated
Comment on lines 395 to 396
Copy link
Copy Markdown
Member

@hashhar hashhar Jan 13, 2022

Choose a reason for hiding this comment

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

nitpicking - this is technically true but a side effect of the fact that stats become missing. i.e. there might be future cost-based optimisations which might not be dependent on stats (or we could generate some rough stats during runtime) and then this statement would be false.

Copy link
Copy Markdown
Contributor Author

@rosewms rosewms Jan 13, 2022

Choose a reason for hiding this comment

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

Thanks @hashhar, thats good info to have.

@mosabua and @m57lyra - should I future proof for the above use cases or assume that we'll update the definition as needed?

Copy link
Copy Markdown
Contributor Author

@rosewms rosewms Jan 13, 2022

Choose a reason for hiding this comment

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

I could do a minor edit to say this or something similar:

Set to false to disable statistics. Disabling statistics
means that :doc:/optimizer/cost-based-optimizations can
not make smart decisions about the query plan.

Copy link
Copy Markdown
Member

@Ordinant Ordinant Jan 13, 2022

Choose a reason for hiding this comment

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

+1 on this minor edit.

Copy link
Copy Markdown
Contributor Author

@rosewms rosewms Jan 14, 2022

Choose a reason for hiding this comment

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

Implemented edit.

@rosewms rosewms requested a review from Ordinant January 14, 2022 17:48
@mosabua mosabua removed the WIP label Jan 14, 2022
@electrum electrum merged commit 50f0857 into trinodb:master Jan 14, 2022
@github-actions github-actions bot added this to the 369 milestone Jan 14, 2022
@mosabua mosabua mentioned this pull request Jan 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@electrum electrum electrum approved these changes

@mosabua mosabua mosabua approved these changes

@hashhar hashhar hashhar approved these changes

@kbajda kbajda Awaiting requested review from kbajda

@raunaqmorarka raunaqmorarka Awaiting requested review from raunaqmorarka

@m57lyra m57lyra Awaiting requested review from m57lyra

@Ordinant Ordinant Awaiting requested review from Ordinant

Assignees

No one assigned

Labels

cla-signed docs

Milestone

369

Development

Successfully merging this pull request may close these issues.

6 participants

@rosewms @electrum @mosabua @m57lyra @Ordinant @hashhar