HIVE-27984: Support backward compatibility of hms thrift struct about column stats #4984

zhangbutao · 2024-01-08T06:34:21Z

What changes were proposed in this pull request?

Why are the changes needed?

Please see details from HIVE-27984.
HIVE-22046 indroduced the must required engine field in hms thrift api, which broken the backward compatibility. This lead to hive3 hms thrift client can not call the hive4 column stats api.

This PR try to make engine field optional and then this will reduce the incompatibility issues.

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

Test with local cluster. And this change has no negative effect to Hive.

zhangbutao · 2024-01-08T06:39:15Z

standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift

@@ -725,7 +725,7 @@ struct SetPartitionsStatsRequest {
 2: optional bool needMerge, //stats need to be merged with the existing stats
 3: optional i64 writeId=-1,         // writeId for the current query that updates the stats
 4: optional string validWriteIdList, // valid write id list for the table for which this struct is being sent
-5: required string engine //engine creating the current request
+5: optional string engine = "hive" //engine creating the current request


Make the engine field optional, and give it a default value hive which is consistent with the current existing code logic.

could we set the engine to default value hive without changing it to optional?

That doesn't work. required will enforce check engine field in construction method though it has a default value. For some others framework(Trino) will use this class SetPartitionsStatsRequest but don't want to care the engine field, and they just reuse hive stats.

optional has no any problem in current hive code. only need partial trival changes in HiveMetaStoreClient.java. I think it is safe. :)

Though I am not familiar with Thrift, I expect the following behaviors.

Client protocol HMS version Behavior

Hive 3 (no engine is set) Hive 4 w/o HIVE-27984 Error

Hive 4 w/o HIVE-27984 Hive 4 w/o HIVE-27984 The engine type specified by the client is used

Hive 4 w/ HIVE-27984 Hive 4 w/o HIVE-27984 The engine type specified by the client is used. Error is a client nullified engine explicitly?

Hive 3 (no engine is set) Hive 4 w/ HIVE-27984 engine=hive

Hive 4 w/o HIVE-27984 Hive 4 w/ HIVE-27984 The engine type specified by the client is used

Hive 4 w/ HIVE-27984 Hive 4 w/ HIVE-27984 The engine type specified by the client is used. engine-hive if a client nullifies engine explicitly?

Yes, you are almost right. To be more specific, for example, Trino will get hms table column statistics by its thrift hms client which is compatible with hive3.
https://github.com/trinodb/trino/blob/057f2d435d6461855ff0f904dbd675b39db97d1e/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java#L331-L336

But TableStatsRequest in Hive4 need pass engine through constructor function, and the engine field is required, therefore:

for hive3 thrift client, this api call will fail due to the missed engine field if HMS server doesn't have this PR, As there is no place for hive3 thrift client to pass engine field. But this api call will sucessfully if HMS has this PR(engine field is optional and has the defalut value).

for hive4 thrift client without this PR, you also must pass engine field in TableStatsRequest to call this api correctly no matter if HMS server has this PR. Because engine field is needed in TableStatsRequest constructor function.

for hive4 thrift client with this PR. Whether HMS server have this PR or not
, you can call the api sucessfully without pass engine as the engine field is not mandatory in TableStatsRequest constructor function, and you can explicitly reset the engine fileld by TableStatsRequest::setEngine if you don't want the default hive engine column statistics. But i think most framework(like trino) will want reuse hive engine column statistics.

Sounds a little verbose.. :) In one word, this PR can make hive3 thrift hms client worked well with hive4 hms when calling statistics related api. I have tested this PR with Trino, and i think it is good.

Thanks for elaborating the behaviors. I'd say it is flexible and less surprising for people.

pan3793 · 2024-01-09T02:09:27Z

@zhangbutao hi, have you observed backward compatibility issues with Hive 2.3 client and Hive 4 HMS? also cc @sunchao

zhangbutao · 2024-01-09T02:32:26Z

@zhangbutao hi, have you observed backward compatibility issues with Hive 2.3 client and Hive 4 HMS? also cc @sunchao

@pan3793 I think it depends your use case, for this change, TrinoDB&StarRocks will use its customized thrift client and call ThriftHiveMetastore$Client.recv_get_table_statistics_re to fetch hive tables column stats, so it will occur compatibility issues. But as far as i know, Spark won't call this api to get hive tables column stats, so it is safe for Spark to integrate Hive4 using its embedded Hive2.3 client.
BTW, we have used Spark3 to query hive4 tables for a long time, and we have not found any compatibility issues.

pan3793 · 2024-01-09T02:36:17Z

@zhangbutao great! thanks for such information

zhangbutao · 2024-01-17T10:08:25Z

Can i request your review? @ayushtkn @dengzhhu653 @deniskuzZ
Thx.

… column stats

sonarcloud · 2024-01-22T17:23:10Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

okumin

Thanks for handling this issue!

okumin · 2024-01-26T13:50:24Z

standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift

@@ -725,7 +725,7 @@ struct SetPartitionsStatsRequest {
 2: optional bool needMerge, //stats need to be merged with the existing stats
 3: optional i64 writeId=-1,         // writeId for the current query that updates the stats
 4: optional string validWriteIdList, // valid write id list for the table for which this struct is being sent
-5: required string engine //engine creating the current request
+5: optional string engine = "hive" //engine creating the current request


Though I am not familiar with Thrift, I expect the following behaviors.

Client protocol HMS version Behavior

Hive 3 (no engine is set) Hive 4 w/o HIVE-27984 Error

Hive 4 w/o HIVE-27984 Hive 4 w/o HIVE-27984 The engine type specified by the client is used

Hive 4 w/ HIVE-27984 Hive 4 w/o HIVE-27984 The engine type specified by the client is used. Error is a client nullified engine explicitly?

Hive 3 (no engine is set) Hive 4 w/ HIVE-27984 engine=hive

Hive 4 w/o HIVE-27984 Hive 4 w/ HIVE-27984 The engine type specified by the client is used

Hive 4 w/ HIVE-27984 Hive 4 w/ HIVE-27984 The engine type specified by the client is used. engine-hive if a client nullifies engine explicitly?

… column stats (apache#4984)(Butao Zhang, reviewed by okumin, Zhihua Deng)

github-actions bot requested a review from klcopp January 8, 2024 06:34

asf-ci-hive added the tests pending label Jan 8, 2024

zhangbutao commented Jan 8, 2024

View reviewed changes

asf-ci-hive added tests failed and removed tests pending labels Jan 8, 2024

zhangbutao force-pushed the HIVE-27984 branch from f314bbf to e5f0074 Compare January 8, 2024 07:36

zhangbutao removed the request for review from klcopp January 8, 2024 07:40

asf-ci-hive added tests pending tests passed and removed tests failed tests pending labels Jan 8, 2024

zhangbutao added 2 commits January 22, 2024 12:49

HIVE-27984: Support backward compatibility of hms thrift struct about…

b98176e

… column stats

set default value 'hive'

c0551e5

zhangbutao force-pushed the HIVE-27984 branch from e5f0074 to c0551e5 Compare January 22, 2024 05:31

github-actions bot requested a review from klcopp January 22, 2024 05:31

asf-ci-hive added tests pending tests unstable and removed tests passed tests pending tests unstable labels Jan 22, 2024

asf-ci-hive added tests passed and removed tests pending labels Jan 22, 2024

okumin approved these changes Jan 26, 2024

View reviewed changes

dengzhhu653 approved these changes Jan 31, 2024

View reviewed changes

zhangbutao mentioned this pull request Jan 31, 2024

Add thrift parameters to compatible with Apache Hive 4.0 trinodb/trino-hive-apache#43

Closed

zhangbutao merged commit 6ab9b14 into apache:master Feb 1, 2024
5 checks passed

tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Feb 9, 2024

HIVE-27984: Support backward compatibility of hms thrift struct about…

4ab003f

… column stats (apache#4984)(Butao Zhang, reviewed by okumin, Zhihua Deng)

dengzhhu653 pushed a commit to dengzhhu653/hive that referenced this pull request Mar 7, 2024

HIVE-27984: Support backward compatibility of hms thrift struct about…

864d8a2

… column stats (apache#4984)(Butao Zhang, reviewed by okumin, Zhihua Deng)

hashhar mentioned this pull request Mar 18, 2024

Can't ANALYZE stats against Hive v3 trinodb/trino#8548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-27984: Support backward compatibility of hms thrift struct about column stats #4984

HIVE-27984: Support backward compatibility of hms thrift struct about column stats #4984

zhangbutao commented Jan 8, 2024 •

edited

Loading

zhangbutao Jan 8, 2024

dengzhhu653 Jan 23, 2024

zhangbutao Jan 23, 2024

okumin Jan 26, 2024 •

edited

Loading

zhangbutao Jan 30, 2024

okumin Jan 31, 2024

pan3793 commented Jan 9, 2024

zhangbutao commented Jan 9, 2024

pan3793 commented Jan 9, 2024

zhangbutao commented Jan 17, 2024

sonarcloud bot commented Jan 22, 2024

okumin left a comment

okumin Jan 26, 2024 •

edited

Loading

Client protocol	HMS version	Behavior
Hive 3 (no engine is set)	Hive 4 w/o HIVE-27984	Error
Hive 4 w/o HIVE-27984	Hive 4 w/o HIVE-27984	The engine type specified by the client is used
Hive 4 w/ HIVE-27984	Hive 4 w/o HIVE-27984	The engine type specified by the client is used. Error is a client nullified `engine` explicitly?
Hive 3 (no engine is set)	Hive 4 w/ HIVE-27984	engine=hive
Hive 4 w/o HIVE-27984	Hive 4 w/ HIVE-27984	The engine type specified by the client is used
Hive 4 w/ HIVE-27984	Hive 4 w/ HIVE-27984	The engine type specified by the client is used. `engine-hive` if a client nullifies `engine` explicitly?

HIVE-27984: Support backward compatibility of hms thrift struct about column stats #4984

HIVE-27984: Support backward compatibility of hms thrift struct about column stats #4984

Conversation

zhangbutao commented Jan 8, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

zhangbutao Jan 8, 2024

Choose a reason for hiding this comment

dengzhhu653 Jan 23, 2024

Choose a reason for hiding this comment

zhangbutao Jan 23, 2024

Choose a reason for hiding this comment

okumin Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

zhangbutao Jan 30, 2024

Choose a reason for hiding this comment

okumin Jan 31, 2024

Choose a reason for hiding this comment

pan3793 commented Jan 9, 2024

zhangbutao commented Jan 9, 2024

pan3793 commented Jan 9, 2024

zhangbutao commented Jan 17, 2024

sonarcloud bot commented Jan 22, 2024

Quality Gate passed

okumin left a comment

Choose a reason for hiding this comment

okumin Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

zhangbutao commented Jan 8, 2024 •

edited

Loading

okumin Jan 26, 2024 •

edited

Loading

okumin Jan 26, 2024 •

edited

Loading