HIVE-28654: MSCK repair fails for default partition when partition column is of numeric type by Aggarwal-Raghav · Pull Request #5568 · apache/hive

Aggarwal-Raghav · 2024-12-02T16:58:51Z

What changes were proposed in this pull request?

HIVE-28654

Why are the changes needed?

To ensure msck repair command works when default partition "HIVE_DEFAULT_PARTITION" is present

Does this PR introduce any user-facing change?

NO

Is the change a dependency upgrade?

NO

How was this patch tested?

Using q files (present in PR)

mvn test -Dtest=TestMiniLlapLocalCliDriver -Dqfile=msck_repair_9.q -Drat.skip -Dtest.output.overwrite
mvn test -Dtest=TestMiniLlapLocalCliDriver -Dqfile=msck_repair_8.q -Drat.skip -Dtest.output.overwrite

ngsg · 2024-12-03T09:58:10Z


  public static String getNormalisedPartitionValue(String partitionValue, String type) {

+    if (partitionValue.equals("__HIVE_DEFAULT_PARTITION__"))


I have a quick question here. Does the patch solve the issue regardless of the value of hive.exec.default.partition.name? If not, I think it would be better to retreive the default partition name from HiveConf rather than using __HIVE_DEFAULT_PARTITION__.

@ngsg, thanks for reviewing the PR. Yes, the issue is regarding hive.exec.default.partition.name. As hive-common is not a dependency in standalone-metastore/metastore-server/pom.xml , i think it better to use MetaConf as done in getPartitionListByFilterExp

hive/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreChecker.java

Line 270 in 2ff3ed6

MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME), results);

Indhumathi27 · 2024-12-05T04:33:06Z

  }

-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type, String defaultPartName) {


why dont we get and check MetastoreConf.ConfVars.DEFAULTPARTITIONNAME here itself, rather than passing from the caller ?

We would need a conf object to fetch the value of DEFAULTPARTITIONNAME and conf is not attribute of MetaStoreServerUtils class. Therefore, String is passed in the function argument.

Indhumathi27 · 2024-12-05T12:02:16Z

      partColNames.add(fSchema.getName());
    }

+    String defaultPartName = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME);


I think, this solution may not work in all scenarios. Since hive.exec.default.partition.name is session configurable, that means, we can use different default partition names with multiple inserts. And when user does MSCK repair, if we take current configuration value of hive.exec.default.partition.name, it can fail.

Yes, that case the solution won't work.
But IMO, its user's responsibility to create the default partition of same type as that of the partition column type and ensure no other datatype partition column is present.

Consider 2 scenarios where partition column is of INT type:
Scenario1:
User1 sets default partition conf value to "ANOTHER_PARTITION" and user 2 in another beeline session tries to insert data in default partition, then hdfs dir structure will look something like this:

path to table/part=ANOTHER_PARITION path to table/part=__HIVE_DEFAULT_PARTITION__

and running msck repair will never succeed for any user. One user will get this

Caused by: java.lang.NumberFormatException: For input string: "ANOTHER_PARITION"

and other will get this

Caused by: java.lang.NumberFormatException: For input string: "__HIVE_DEFAULT_PARTITION__"

Scenario2:
User set default partition conf value to INT. Lets say 13, so hdfs dir structure looks like this

path to table/part=13 path to table/part=__HIVE_DEFAULT_PARTITION__

and in same session user creates a new table pointing of location of old table and run msck repair then it will fail for __HIVE_DEFAULT_PARTITION__.

Unless the user change the config back to __HIVE_DEFAULT_PARTITION__

Questions that I also have, is going forward what should be the expected behaviour? That the user has to ensure that there shouldn't be 2 partitions of different datatype than that of partition column as shown in scenario 1?

CC @ayushtkn @deniskuzZ -- please provide your insights on this?

Also, there are multiple places where __HIVE_DEFAULT_PARTITION__ is explicitly used instead of the config value. For instance here:

hive/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/FileUtils.java

Line 307 in c27d317

return "__HIVE_DEFAULT_PARTITION__";

I just had a quick look seems like the problem is coming post HIVE-23347, which added this method getNormalisedPartitionValue which does this parsing for numeric values. Before that patch things should have worked & Scenario-1 would have passed as well, with both values being shown in show partitions

This method getNormalisedPartitionValue looks like is valid only for numeric values to sort out some 0s or so. Maybe we can just have a numeric check & return rather than this default partition value matching

if(!NumberUtils.isParsable(partitionValue)) { return partitionValue; }

If we check the regular case without MSCK for scenario-1, the show partitions would show both the values, I tried locally

+------------------------------------------+ | partition | +------------------------------------------+ | month=12/day=2 | | month=12/day=3 | | month=12/day=__HIVE_DEFAULT_PARTITION__ | | month=__ANOTHER_PARTITION__/day=9 | | month=__HIVE_DEFAULT_PARTITION__/day=3 | +------------------------------------------+ 5 rows selected (0.052 seconds) 0: jdbc:hive2://localhost:10000/>

So, post MSCK also we should show both those values. So, I believe we should just check whether the value is numeric or not, if not, just return the value as is..

@ayushtkn Thanks for looking into this. Yes, I have update the PR based on your suggestion

…lumn is of numeric type

okumin · 2024-12-09T09:41:45Z

+SHOW PARTITIONS tbl_y;
+
+DROP TABLE tbl_x;
+DROP TABLE tbl_y;


Thanks for adding great test cases

okumin · 2024-12-09T09:43:13Z


+    if (!NumberUtils.isParsable(partitionValue)) {
+      return partitionValue;
+    }


I slightly suspect this can skip the case of decimal.
https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/math/NumberUtils.html#isParsable(java.lang.String)

@okumin , thanks for the input. I think there is already a UT for this and passing post the patch. Please check

hive/standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java

Line 965 in 0e75930

assertEquals("1.01", MetaStoreServerUtils.getNormalisedPartitionValue("0001.0100", "decimal"));

Let me know if I am missing something, but NumberUtils.isParsable(partitionValue) will return true for a decimal and later in same function it will be handled.

hive/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java

Line 1653 in 0e75930

return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();

I originally thought it could not work when Double.parseDouble can not decode it but new BigDecimal can decode it. An example in my mind was a huge number. Looking at the implementation, now I feel the current implementation would work
https://github.com/apache/commons-lang/blob/daa63aa595ea39a69e1b44ac23bc50fdcf2afd62/src/main/java/org/apache/commons/lang3/math/NumberUtils.java#L1794-L1810

sonarqubecloud · 2024-12-09T11:53:50Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ayushtkn

LGTM

asf-ci-hive added tests pending tests unstable and removed tests pending labels Dec 2, 2024

Aggarwal-Raghav force-pushed the HIVE-msck branch from d502d48 to 0eff58b Compare December 3, 2024 04:44

asf-ci-hive added tests pending tests failed and removed tests unstable tests pending labels Dec 3, 2024

ngsg reviewed Dec 3, 2024

View reviewed changes

Aggarwal-Raghav force-pushed the HIVE-msck branch from 0eff58b to 6476ff1 Compare December 4, 2024 06:21

asf-ci-hive added tests pending tests passed and removed tests failed tests pending labels Dec 4, 2024

Indhumathi27 reviewed Dec 5, 2024

View reviewed changes

Aggarwal-Raghav force-pushed the HIVE-msck branch from 6476ff1 to 7ceb72b Compare December 9, 2024 06:08

asf-ci-hive added tests pending tests unstable and removed tests passed tests pending labels Dec 9, 2024

HIVE-28654: MSCK repair fails for default partition when partition co…

ff4994b

…lumn is of numeric type

Aggarwal-Raghav force-pushed the HIVE-msck branch from 7ceb72b to ff4994b Compare December 9, 2024 09:03

okumin reviewed Dec 9, 2024

View reviewed changes

asf-ci-hive added tests pending and removed tests unstable labels Dec 9, 2024

asf-ci-hive added tests passed and removed tests pending labels Dec 9, 2024

okumin approved these changes Dec 9, 2024

View reviewed changes

ayushtkn approved these changes Dec 9, 2024

View reviewed changes

ayushtkn merged commit 3483bc3 into apache:master Dec 10, 2024

Aggarwal-Raghav mentioned this pull request Jul 23, 2025

HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns #5976

Merged

Aggarwal-Raghav deleted the HIVE-msck branch October 2, 2025 15:09


		public static String getNormalisedPartitionValue(String partitionValue, String type) {

		if (partitionValue.equals("__HIVE_DEFAULT_PARTITION__"))

Uh oh!

Conversation

Aggarwal-Raghav commented Dec 2, 2024

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Dec 9, 2024

Quality Gate passed

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants