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

Fallback to an available alter table/partitions API in Hive Metastore 3.x #12771

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Fallback to an available alter table/partitions API in Hive Metastore 3.x #12771

merged 1 commit into from
Dec 2, 2022

Conversation

posulliv
Copy link
Contributor

@posulliv posulliv commented Jun 9, 2022

Description

HiveMetaStore 3.1.2 doesn't have the alter_table_req and alter_partitions_req API requests. This PR adds the ability to fallback to alter_table_with_environment_context and alter_partitions_with_environment_context in case those APIs are not available.

This PR takes the commit from an old open PR (#6180) and updates it for Trino and adds a product test.

I added a new product test as I could not get the testHiveTransactionalTable product test to work with the Apache Hive 3.1.2 HMS since it requires a whole Hadoop environment. Instead, I created a few simple tests to cover this functionality and added them to the HMS_ONLY group which uses an Apache Hive 3.1.2 metastore.

Is this change a fix, improvement, new feature, refactoring, or other?

Fix.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Hive connector.

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

(X ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix writes to ORC ACID tables when the metastore being used is Apache Hive 3.1.2. ({issue}`7310`)

@cla-bot cla-bot bot added the cla-signed label Jun 9, 2022
@findepi
Copy link
Member

findepi commented Jun 9, 2022

This needs a product test with Hive 3 (non-HDP).
@posulliv are you planning on working on this?

@posulliv
Copy link
Contributor Author

posulliv commented Jun 9, 2022

This needs a product test with Hive 3 (non-HDP).

@findepi my understanding is the HMS_ONLY group is run as part of the SuiteHmsOnly product test suite. And this test suite runs with Hive 3 (non hdp). The tests I added are part of the HMS_ONLY group and when I ran this locally I saw them run with Hive 3 (non-HDP).

@posulliv posulliv force-pushed the alter-table-hms-api branch 2 times, most recently from 170d64b to 3b6b4ce Compare June 16, 2022 15:23
@posulliv posulliv requested review from homar and findepi June 28, 2022 17:55
findepi
findepi reviewed Jul 19, 2022
View reviewed changes
...roduct-tests-launcher/src/main/java/io/trino/tests/product/launcher/suite/suites/Suite1.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated
return null;
},
client -> {
client.alterTableWithEnvironmentContext(table.getDbName(), table.getTableName(), table, new EnvironmentContext());
Copy link
Member

Choose a reason for hiding this comment

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

why no transactionId and writeId being passed here here?

this is called from updateTableWriteId method, so seems like writeId is the reason why we are calling HMS.

cc @djsstarburst @losipiuk

Copy link
Member

Choose a reason for hiding this comment

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

I think the reason that there is no writeId passed explicitly here is that there is a writeId in the Table object. I verified that is true by looking at the generated Thrift code.

@bitsondatadev bitsondatadev mentioned this pull request Oct 21, 2022
Closed
@posulliv posulliv force-pushed the alter-table-hms-api branch 4 times, most recently from 9fadc75 to 91cb4f8 Compare November 11, 2022 01:29
@posulliv
Copy link
Contributor Author

@findepi apologies for not keeping on top of this PR. I've resolved conflicts and applied all feedback so its ready for another review whenever you get a chance. Thanks!

@Praveen2112
Copy link
Member

@posulliv Can you please squash the commits so that PTs and test are in same commit

@Praveen2112
Copy link
Member

Can we restrict the commit message length to around 70

@posulliv @abhishekkhanna01
Fallback to an available alter table and alter partitions API in HMS 3.x
9184619 
Hive Metastore 3.1.2 doesn't have the new alter_table_req and
alter_partitions_req API. This commit adds the ability to fallback
to alter_table_with_environment_context and
alter_partitions_with_environment_context in case the new APIs are not
available.

Co-authored-by: Abhishek Khanna <[email protected]>
@posulliv
Copy link
Contributor Author

@Praveen2112 no problem. Squashed into a single commit and restricted line length for commit message to about 70 characters.

djsstarburst
djsstarburst approved these changes Nov 29, 2022
View reviewed changes
Copy link
Member

@djsstarburst djsstarburst left a comment

Choose a reason for hiding this comment

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

This looks ok to me. If it works with the 3.1-series, as shown by the tests, it should be ok.

@Praveen2112 Praveen2112 merged commit 951837f into trinodb:master Dec 2, 2022
@github-actions github-actions bot added this to the 404 milestone Dec 2, 2022
@posulliv posulliv deleted the alter-table-hms-api branch December 2, 2022 14:25
@colebow colebow mentioned this pull request Dec 14, 2022
Merged
@hackeryang hackeryang mentioned this pull request Apr 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djsstarburst djsstarburst djsstarburst approved these changes

@Praveen2112 Praveen2112 Praveen2112 approved these changes

@homar homar homar approved these changes

@findepi findepi Awaiting requested review from findepi

Assignees
No one assigned
Labels
cla-signed
Milestone
405
Development

Successfully merging this pull request may close these issues.

Insert into Hive transactional table fails with HMS 3.1.2: Invalid method name alter_table_req
5 participants
@posulliv @findepi @Praveen2112 @homar @djsstarburst