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

Document table statistics property for Hive connector #10525

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

rosewms
Copy link
Contributor

@rosewms rosewms commented Jan 10, 2022

No description provided.

Copy link
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

docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
Copy link
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

docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved
Copy link
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.

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
@@ -388,6 +388,11 @@ Property Name Description
``hive.query-partition-filter-required`` Set to ``true`` to force a query to use a partition filter. ``false``
You can use the ``query_partition_filter_required`` catalog
session property for temporary, catalog specific use.

``hive.statistics_enabled`` Disable :doc:`/optimizer/statistics`, and therefore ``true``
Copy link
Contributor

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.

Copy link
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 on lines 395 to 396
Set to ``false`` to disable statistics, this also disables
:doc:`/optimizer/cost-based-optimizations`.
Copy link
Member

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
Contributor Author

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
Contributor Author

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
Member

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
Contributor Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants