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.

Sign up for GitHub

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

Jump to bottom

[FLINK-12237][hive]Support Hive table stats related operations in HiveCatalog #8636

Closed
wants to merge 6 commits into from
Closed

[FLINK-12237][hive]Support Hive table stats related operations in HiveCatalog #8636

wants to merge 6 commits into from

Conversation

zjuwangg
Copy link
Contributor

@zjuwangg zjuwangg commented Jun 5, 2019 •
edited

What is the purpose of the change

  • This pull request makes HiveCatalog support Hive table stats related operations.

Brief change log

  • Support Hive table stats related operations in HiveCatalog

Verifying this change

This change added tests and can be verified as follows:

  • Added test that validates that function in CatalogBaseTest

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): ( no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no )
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: ( no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (not applicable)

@zjuwangg
Copy link
Contributor Author

zjuwangg commented Jun 5, 2019

cc @bowenli86 @lirui-apache @xuefuz to review this pr. More test cases are needed to consider.

@flinkbot
Copy link
Collaborator

flinkbot commented Jun 5, 2019 •
edited

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

  • ✅ 1. The [description] looks good.
  • ✅ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ✅ 4. The change fits into the overall [architecture].
  • ✅ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

bowenli86
bowenli86 reviewed Jun 5, 2019
View reviewed changes
Copy link
Member

@bowenli86 bowenli86 left a comment •
edited

Choose a reason for hiding this comment

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

Thanks @zjuwangg for your PR. I just roughly went thru it really quickly, and found it's too big and hard to review and question on. Given that stats is a big area, can you break the PR (as well as the JIRA) down into smaller ones? to 1) make it easy to review 2) allow you to add enough test coverage for each new functionality.

I'd recommend to open 1st PR for table stats and all the stats type conversions, 2nd for partition stats, and 3rd for table/partition column stats. 2nd and 3rd PR won't be blocked since you can always base them on the 1st.

...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...k-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/util/HiveCatalogUtil.java Outdated Show resolved Hide resolved
...k-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/util/HiveCatalogUtil.java Outdated Show resolved Hide resolved
xuefuz
xuefuz reviewed Jun 5, 2019
View reviewed changes
Copy link
Contributor

@xuefuz xuefuz 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 the contribution. I left some minor comments.

@zjuwangg
Copy link
Contributor Author

zjuwangg commented Jun 6, 2019

Thanks for @bowenli86 and @xuefuz detailed review, I'll address comments later and break the PR into smaller pieces.

@zjuwangg
Copy link
Contributor Author

ccc @xuefuz again to review.

bowenli86
bowenli86 reviewed Jun 14, 2019
View reviewed changes
Copy link
Member

@bowenli86 bowenli86 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 the PR! I left some comments

One issue I found is that, APIs that alter stats will first convert existing table stats from string to long in needToUpdateStatistics() and then convert new ones from long to string in updateStatisticsParameters(). Can we just convert new stats from long to string at the first place, and just compare and update the strings if needed?

...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Show resolved Hide resolved
...k-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTestBase.java Outdated Show resolved Hide resolved
...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...k-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTestBase.java Outdated Show resolved Hide resolved
...k-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTestBase.java Outdated Show resolved Hide resolved
@bowenli86
Copy link
Member

@zjuwangg can you rebase the PR?

1 similar comment
@bowenli86
Copy link
Member

@zjuwangg can you rebase the PR?

@zjuwangg
Copy link
Contributor Author

Thanks for your guys' detailed review. @bowenli86 @lirui-apache.
I'll address the comments after FLINK-12807 merged first.

bowenli86
bowenli86 reviewed Jun 25, 2019
View reviewed changes
Copy link
Member

@bowenli86 bowenli86 left a comment

Choose a reason for hiding this comment

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

can you rebase to the latest master?

...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
...tors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java Outdated Show resolved Hide resolved
@bowenli86
Copy link
Member

The last commit as "Merge branch 'master' into FLINK-12237" is not right. Instead of merging master into dev branch in a big chunk, it's better for us to rebase dev branch to latest master. Otherwise the changed files would include whatever you merged from master, not just showing what you modified in this PR.

I'd recommend revert that commit, rebase to latest master, and force push this PR.

@zjuwangg
Copy link
Contributor Author

rebase and force to push

@zjuwangg
Copy link
Contributor Author

@bowenli86 Sorry a little late to update this pr, please review again when you have time.

bowenli86
bowenli86 reviewed Jun 27, 2019
View reviewed changes
Copy link
Member

@bowenli86 bowenli86 left a comment

Choose a reason for hiding this comment

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

Nice job! LGTM overall, with just a few minor comments

...link-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/client/HiveShimV1.java

@Override
public void alterTable(IMetaStoreClient client, String databaseName, String tableName, Table table) throws InvalidOperationException, MetaException, TException {
// For Hive-1.2.1, we need to tell HMS not to update stats. Otherwise, the stats we put in the table
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Can you also add comments to explain how Hive-2.3.4 handles it too? Will Hive 2.3.4 just not update any table stats in parameters when client.alter_table is invoked?

flink-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTest.java Show resolved Hide resolved
flink-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/CatalogTest.java Show resolved Hide resolved
@bowenli86
Copy link
Member

LGTM, thanks for the contribution！

@flinkbot approve all

Merging

@asfgit asfgit closed this in 6d7a15e Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xuefuz xuefuz xuefuz left review comments

@bowenli86 bowenli86 bowenli86 left review comments

@lirui-apache lirui-apache lirui-apache left review comments

Assignees
No one assigned
Labels
component=TableSQL/API review=description?
Projects
None yet
Milestone
No milestone
6 participants
@zjuwangg @flinkbot @bowenli86 @xuefuz @lirui-apache @rmetzger