You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "vikramahuja1001 (via GitHub)" <gi...@apache.org> on 2025/07/15 04:49:16 UTC

[PR] Fix MSCK Repair adding invalid partitions [hive]

vikramahuja1001 opened a new pull request, #5976:
URL: https://github.com/apache/hive/pull/5976

   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
     2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
     3. Ensure you have added or run the appropriate tests for your PR: 
     4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX:  Your PR title ...'.
     5. Be sure to keep the PR description updated to reflect all changes.
     6. Please write your PR title to summarize what this PR proposes.
     7. If possible, provide a concise example to reproduce the issue for a faster review.
   
   -->
   
   ### What changes were proposed in this pull request?
   <!--
   Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. 
   If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
     1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
     2. If you fix some SQL features, you can provide some references of other DBMSes.
     3. If there is design documentation, please add the link.
     4. If there is a discussion in the mailing list, please add the link.
   -->
   
   
   ### Why are the changes needed?
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you propose a new API, clarify the use case for a new API.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   
   
   ### Does this PR introduce _any_ user-facing change?
   <!--
   Note that it means *any* user-facing change including all aspects such as the documentation fix.
   If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
   If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
   If no, write 'No'.
   -->
   
   ### How was this patch tested?
   <!--
   If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
   If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
   If tests were not added, please describe why they were not added and/or why it was difficult to add.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2206959673


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,41 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if (!NumberUtils.isParsable(partitionValue) && !type.equalsIgnoreCase("string")

Review Comment:
   Changed to used an enum



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210375241


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   can non-number be anything else than string type?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
         return partitionValue;
     }
   
     if (Objects.equals(partitionValue, MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
         // This is the special partition name for NULL values. It should never be parsed.
         return partitionValue;
     }
   
     // 2. If the type is not numeric, no normalization is needed.
     String normalisedType = ColumnType.getTypeName(type);
     if (!ColumnType.numericTypes.contains(normalisedType)) {
         return partitionValue;
     }
   
     // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
         switch (normalisedType) {
             case ColumnType.TINYINT_TYPE_NAME:
             case ColumnType.SMALLINT_TYPE_NAME:
             case ColumnType.INT_TYPE_NAME:
                 return Integer.toString(Integer.parseInt(partitionValue));
             case ColumnType.BIGINT_TYPE_NAME:
                 return Long.toString(Long.parseLong(partitionValue));
             case ColumnType.FLOAT_TYPE_NAME:
                 return Float.toString(Float.parseFloat(partitionValue));
             case ColumnType.DOUBLE_TYPE_NAME:
                 return Double.toString(Double.parseDouble(partitionValue));
             case ColumnType.DECIMAL_TYPE_NAME:
                 // Removes trailing zeros (e.g., 1.200 -> 1.2)
                 return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
             default:
                 // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                 break;
         }
     } catch (NumberFormatException e) {
         // 4. Handle cases where the value cannot be parsed as the expected number type.
         String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
         if ("throw".equals(validationMode)) {
             LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                     "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
             throw e;
         } else if ("skip".equals(validationMode)) {
             LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
             return null; // Signals the caller to skip this partition.
         }
     }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210390845


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   i don't understand this change:
   
   if the partitionValue is not a number but type is String, we'll attempt to parse the number in partitionValue - doesn't make any sense
   
   DEFAULTPARTITIONNAME only applies to String type



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218358644


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   > INSERT INTO tbl_x (id, name, month) values(3, 'ccc', 12);
   
   PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)
   
   > SET hive.exec.default.partition.name=ANOTHER_PARTITION;
   > INSERT INTO tbl_x (id, name, day) values(4, 'ddd', 3);
   
   PARTITION(month=ANOTHER_PARTITION,day=3)
   
   what happens to `PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)` data?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2209438361


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,77 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public enum PartitionDataType {

Review Comment:
   why not use `org.apache.hadoop.hive.metastore.ColumnType.getTypeName()`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2232818285


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   @deniskuzZ, in the above scenario that you described:
   `INSERT INTO tbl_x (id, name, month) values(3, 'ccc', 12);` -> PARTITION(month=12,day=HIVE_DEFAULT_PARTITION)
   `SET hive.exec.default.partition.name=ANOTHER_PARTITION;`
   `INSERT INTO tbl_x (id, name, day) values(4, 'ddd', 3);` -> PARTITION(month=ANOTHER_PARTITION,day=3)
   `MSCK REPAIR TABLE tbl_x;`
   
   hive.exec.default.partition.name is an exactly one value. So at one point of time only one one value is valid, rest all the other values are invalid. So when MSCK repair is ran , `hive.exec.default.partition.name` has been already set to `ANOTHER_PARTITION` which makes the  HIVE_DEFAULT_PARTITION not a valid partition anymore.
   
   Since by default `hive.msck.path.validation` is set to `throw`,  msck repair will throw an exception as it encounters a invalid partition (month=12,day=HIVE_DEFAULT_PARTITION) and will fail the whole operation without adding any new partition to the HMS with the exception: `Invalid partition value: Cannot parse HIVE_DEFAULT_PARTITION as type int. Failing MSCK. Set hive.msck.path.validation=skip to ignore invalid partitions` 
   
   But when the user sets `hive.msck.path.validation=skip` at  the session level, msck repair operation will succeed but it will skip all the invalid partitions by adding a warning log (month=12,day=HIVE_DEFAULT_PARTITION in this case) and will only add the valid partitions in that case((month=ANOTHER_PARTITION,day=3).
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2233897961


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,50 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {

Review Comment:
   nit: remove unused import statement as well.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3124288175

   @vikramahuja1001, Can you add a new q file for hive.msck.path.validation = ignore. as there is none at the moment. It will help in more coverage.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3126943606

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [4 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210301930


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreChecker.java:
##########
@@ -444,7 +444,12 @@ void findUnknownPartitions(Table table, Set<Path> missingPartDirs,
     for (Path partPath : missingPartDirs) {
       FileSystem fs = partPath.getFileSystem(conf);
       String partitionName = getPartitionName(fs.makeQualified(tablePath),
-          partPath, partColNames, partitionColToTypeMap);
+          partPath, partColNames, partitionColToTypeMap, conf);
+      if (partitionName == null) {
+        // Skip this partition if there is some issue in the partition validation
+        LOG.warn("Skipping partition : " + partPath.getName());
+        continue;
+      }
       LOG.debug("PartitionName: " + partitionName);
 
       if (partitionName != null) {

Review Comment:
   why check for null, if we skip the iteration when `partitionName == null`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
         return partitionValue;
     }
   
     if (Objects.equals(partitionValue, 
       MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
         // This is the special partition name for NULL values. It should never be parsed.
         return partitionValue;
     }
   
     // 2. If the type is not numeric, no normalization is needed.
     String normalisedType = ColumnType.getTypeName(type);
     if (!ColumnType.numericTypes.contains(normalisedType)) {
         return partitionValue;
     }
   
     // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
         switch (normalisedType) {
             case ColumnType.TINYINT_TYPE_NAME:
             case ColumnType.SMALLINT_TYPE_NAME:
             case ColumnType.INT_TYPE_NAME:
                 return Integer.toString(Integer.parseInt(partitionValue));
             case ColumnType.BIGINT_TYPE_NAME:
                 return Long.toString(Long.parseLong(partitionValue));
             case ColumnType.FLOAT_TYPE_NAME:
                 return Float.toString(Float.parseFloat(partitionValue));
             case ColumnType.DOUBLE_TYPE_NAME:
                 return Double.toString(Double.parseDouble(partitionValue));
             case ColumnType.DECIMAL_TYPE_NAME:
                 // Removes trailing zeros (e.g., 1.200 -> 1.2)
                 return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
             default:
                 // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                 break;
         }
     } catch (NumberFormatException e) {
         // 4. Handle cases where the value cannot be parsed as the expected number type.
         String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
         if ("throw".equals(validationMode)) {
             LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                     "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
             throw e;
         } else if ("skip".equals(validationMode)) {
             LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
             return null; // Signals the caller to skip this partition.
         }
     }
     return partitionValue;
   }
   ````



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
         return partitionValue;
     }
   
     if (Objects.equals(partitionValue, MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
         // This is the special partition name for NULL values. It should never be parsed.
         return partitionValue;
     }
   
     // 2. If the type is not numeric, no normalization is needed.
     String normalisedType = ColumnType.getTypeName(type);
     if (!ColumnType.numericTypes.contains(normalisedType)) {
         return partitionValue;
     }
   
     // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
         switch (normalisedType) {
             case ColumnType.TINYINT_TYPE_NAME:
             case ColumnType.SMALLINT_TYPE_NAME:
             case ColumnType.INT_TYPE_NAME:
                 return Integer.toString(Integer.parseInt(partitionValue));
             case ColumnType.BIGINT_TYPE_NAME:
                 return Long.toString(Long.parseLong(partitionValue));
             case ColumnType.FLOAT_TYPE_NAME:
                 return Float.toString(Float.parseFloat(partitionValue));
             case ColumnType.DOUBLE_TYPE_NAME:
                 return Double.toString(Double.parseDouble(partitionValue));
             case ColumnType.DECIMAL_TYPE_NAME:
                 // Removes trailing zeros (e.g., 1.200 -> 1.2)
                 return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
             default:
                 // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                 break;
         }
     } catch (NumberFormatException e) {
         // 4. Handle cases where the value cannot be parsed as the expected number type.
         String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
         if ("throw".equals(validationMode)) {
             LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                     "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
             throw e;
         } else if ("skip".equals(validationMode)) {
             LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
             return null; // Signals the caller to skip this partition.
         }
     }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
       // 1. Handle simple exit cases first.
       if (type == null) {
           return partitionValue;
       }
   
       final String defaultPartitionName = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME);
       if (Objects.equals(partitionValue, defaultPartitionName)) {
           // This is the special partition name for NULL values. It should never be parsed.
           return partitionValue;
       }
   
       // 2. If the type is not numeric, no normalization is needed.
       String normalisedType = ColumnType.getTypeName(type);
       if (!ColumnType.NumericTypes.contains(normalisedType)) {
           return partitionValue;
       }
   
       // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
           try {
               switch (normalisedType) {
                   case ColumnType.TINYINT_TYPE_NAME:
                   case ColumnType.SMALLINT_TYPE_NAME:
                   case ColumnType.INT_TYPE_NAME:
                       return Integer.toString(Integer.parseInt(partitionValue));
                   case ColumnType.BIGINT_TYPE_NAME:
                       return Long.toString(Long.parseLong(partitionValue));
                   case ColumnType.FLOAT_TYPE_NAME:
                       return Float.toString(Float.parseFloat(partitionValue));
                   case ColumnType.DOUBLE_TYPE_NAME:
                       return Double.toString(Double.parseDouble(partitionValue));
                   case ColumnType.DECIMAL_TYPE_NAME:
                       // Removes trailing zeros (e.g., 1.200 -> 1.2)
                       return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
                   default:
                       // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                       break;
               }
           } catch (NumberFormatException e) {
               // 4. Handle cases where the value cannot be parsed as the expected number type.
               String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
               if ("throw".equals(validationMode)) {
                   LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                           "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
                   throw e;
               } else if ("skip".equals(validationMode)) {
                   LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
                   return null; // Signals the caller to skip this partition.
               }
           }
       return partitionValue;
     }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210358193


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) || type == null) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
-
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      switch (type.toLowerCase()) {
+        case ColumnType.TINYINT_TYPE_NAME:
+        case ColumnType.SMALLINT_TYPE_NAME:
+        case ColumnType.INT_TYPE_NAME:
+          return Integer.toString(Integer.parseInt(partitionValue));
+        case ColumnType.BIGINT_TYPE_NAME:
+          return Long.toString(Long.parseLong(partitionValue));
+        case ColumnType.FLOAT_TYPE_NAME:
+          return Float.toString(Float.parseFloat(partitionValue));
+        case ColumnType.DOUBLE_TYPE_NAME:
+          return Double.toString(Double.parseDouble(partitionValue));
+        case ColumnType.DECIMAL_TYPE_NAME:
+          return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+        default:
+          break;
+      }
+    } catch (NumberFormatException e) {

Review Comment:
   there is no point in checking if value is a number and then handling the exception



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210282588


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   why `tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__` is gone?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210292063


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -100,8 +100,40 @@ POSTHOOK: type: SHOWPARTITIONS
 POSTHOOK: Input: default@tbl_y
 month=12/day=2
 month=12/day=3
-month=12/day=__HIVE_DEFAULT_PARTITION__

Review Comment:
   same here



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2233131268


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   @deniskuzZ @vikramahuja1001 , I'll review this PR by Monday. Currently Out of station.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2221434849


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   isn't it a regression, since it was working before? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2224662016


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   @vikramahuja1001 , i would recomment you to check the disucssion in this thread https://github.com/apache/hive/pull/5568#discussion_r1871258402 . Based on this discussion only I went ahead allowing any string partition name.
   
   **My understanding is this**: If user is adding a string partition name in non-string column then its user's responsibility and he should be consistent with the default partition name for that table. As DEFAULT PARTITION is configurable at session level, we can't stop user from changing it / playing around.
   
   Imposing checks done in your current PR will be a breaking change b/w 4.0.1 and next hive release version and forcing users to set this on cluster level `hive.msck.path.validation=skip` from what I can infer from modified msck_repair_9.q



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2233898338


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,50 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
+    // 1. Handle simple exit cases first.
+    if (type == null) {
       return partitionValue;
     }
-
-    LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");

Review Comment:
   Can we use string formatter here as sona type is also flagging it?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2206748249


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,41 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if (!NumberUtils.isParsable(partitionValue) && !type.equalsIgnoreCase("string")

Review Comment:
   don't we have constants or enum for type like string?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2206958951


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,41 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if (!NumberUtils.isParsable(partitionValue) && !type.equalsIgnoreCase("string")
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
 
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      if (type.equalsIgnoreCase("tinyint")

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210267288


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) || type == null) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
-
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      switch (type.toLowerCase()) {

Review Comment:
   maybe `ColumnType.getTypeName(type)`? this would handle the `type.startsWith("decimal")`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210282588


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   why was this changed?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218358644


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   > INSERT INTO tbl_x (id, name, month) values(3, 'ccc', 12);
   
   PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)
   
   > SET hive.exec.default.partition.name=ANOTHER_PARTITION;
   > INSERT INTO tbl_x (id, name, day) values(4, 'ddd', 3);
   
   PARTITION(month=ANOTHER_PARTITION,day=3)
   
   what happens to `PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)` data after the MSCK REPAIR?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2221564152


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   No regression. It was working like this before HIVE-28654. We are able to add any string partition value to a non string partition column as of now which doesn't make sense. This is a bug induced by HIVE-28654



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2225269017


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   Hey, I have gone through the discussion in the thread [PR5568](https://github.com/apache/hive/pull/5568#discussion_r1871258402) . 
   
   Have gone through both the scenarios that you have mentioned in that discussion.  I believe under no circumstance the user should be allowed to add arbitrary strings for non string partition values by using MSCK repair it can lead to unsafe comparisons down the line. Only DEFAULT PARTITION VALUE should be considered at a time. If not, this leads to various issues like:
   1. **Type Integrity Violation:**
   The core expectation in Hive is that partition values must adhere to the column's defined type. Allowing arbitrary strings in a BIGINT/INT or any other non string column undermines this contract.
   2. Downstream Failures:
   Operations downstream (like queries, filters, and metadata tools) may fail or behave unpredictably when encountering such invalid partition values opening a pandora box of further issues. For instance: [HIVE-29100](https://issues.apache.org/jira/browse/HIVE-29100). We will need to specifically handle such cases now.
   3. Silent Corruption Risk:
   Since MSCK is a metadata recovery tool, silently accepting bad values gives a false sense of correctness, potentially leading to hard-to-debug issues in the future.
   4. Inconsistent Behavior Across Interfaces:
   Hive does not allow such partitions to be added via standard DDL operations (e.g., ALTER TABLE ADD PARTITION) due to strict type checking. This inconsistency between interfaces (DDL vs MSCK) can confuse users and lead to misconfiguration.
   
   We should allow partitions under hive.exec.default.partition.name property and thus a basic validation like is must and the property` hive.msck.path.validation` can decide whether to throw an error for such cases or just log and skip those arbitrary partitions.
   
   The config ` hive.msck.path.validation=skip` can be set as session level. By default I have kept this value as `throw`. No need to set anything at cluster level. 
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3124295980

   **NOTE**: Just want to re-iterate and highlight the breaking change here. Anyway as +1 is there on the PR which means it's ok from dev perspective.
   
   Have give some nit comments for improving code readability which are purely optional. 
   
   **Before this PR:**
   <img width="774" height="480" alt="scenarion2_3_without_patch" src="https://github.com/user-attachments/assets/c8ba93dd-00a3-4006-ac71-22390e1bfa1e" />
   
   **After this PR:**
   <img width="1512" height="749" alt="Screenshot 2025-07-27 at 3 43 39 PM" src="https://github.com/user-attachments/assets/2e477ead-b0fe-4f4d-84e2-ec538a9e8789" />
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
         return partitionValue;
     }
   
     if (Objects.equals(partitionValue, MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
         // This is the special partition name for NULL values. It should never be parsed.
         return partitionValue;
     }
   
     // 2. If the type is not numeric, no normalization is needed.
     String normalisedType = ColumnType.getTypeName(type);
     if (!ColumnType.NumericTypes.contains(normalisedType)) {
         return partitionValue;
     }
   
     // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
         switch (normalisedType) {
             case ColumnType.TINYINT_TYPE_NAME:
             case ColumnType.SMALLINT_TYPE_NAME:
             case ColumnType.INT_TYPE_NAME:
                 return Integer.toString(Integer.parseInt(partitionValue));
             case ColumnType.BIGINT_TYPE_NAME:
                 return Long.toString(Long.parseLong(partitionValue));
             case ColumnType.FLOAT_TYPE_NAME:
                 return Float.toString(Float.parseFloat(partitionValue));
             case ColumnType.DOUBLE_TYPE_NAME:
                 return Double.toString(Double.parseDouble(partitionValue));
             case ColumnType.DECIMAL_TYPE_NAME:
                 // Removes trailing zeros (e.g., 1.200 -> 1.2)
                 return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
             default:
                 // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                 break;
         }
     } catch (NumberFormatException e) {
         // 4. Handle cases where the value cannot be parsed as the expected number type.
         String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
         if ("throw".equals(validationMode)) {
             LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                     "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
             throw e;
         } else if ("skip".equals(validationMode)) {
             LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
             return null; // Signals the caller to skip this partition.
         }
     }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
         return partitionValue;
     }
   
     if (Objects.equals(partitionValue, MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
         // This is the special partition name for NULL values. It should never be parsed.
         return partitionValue;
     }
   
     // 2. If the type is not numeric, no normalization is needed.
     String colType = ColumnType.getTypeName(type);
     if (!ColumnType.numericTypes.contains(colType)) {
         return partitionValue;
     }
   
     // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
         switch (colType) {
             case ColumnType.TINYINT_TYPE_NAME:
             case ColumnType.SMALLINT_TYPE_NAME:
             case ColumnType.INT_TYPE_NAME:
                 return Integer.toString(Integer.parseInt(partitionValue));
             case ColumnType.BIGINT_TYPE_NAME:
                 return Long.toString(Long.parseLong(partitionValue));
             case ColumnType.FLOAT_TYPE_NAME:
                 return Float.toString(Float.parseFloat(partitionValue));
             case ColumnType.DOUBLE_TYPE_NAME:
                 return Double.toString(Double.parseDouble(partitionValue));
             case ColumnType.DECIMAL_TYPE_NAME:
                 // Removes trailing zeros (e.g., 1.200 -> 1.2)
                 return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
         }
     } catch (NumberFormatException e) {
         // 4. Handle cases where the value cannot be parsed as the expected number type.
         String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
         if ("throw".equals(validationMode)) {
             LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                     "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
             throw e;
         } else if ("skip".equals(validationMode)) {
             LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
             return null; // Signals the caller to skip this partition.
         }
     }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2236545891


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,50 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {

Review Comment:
   Sure



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,50 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
+    // 1. Handle simple exit cases first.
+    if (type == null) {
       return partitionValue;
     }
-
-    LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");

Review Comment:
   Sure



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3095448128

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [6 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2225267868


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreChecker.java:
##########
@@ -444,34 +444,37 @@ void findUnknownPartitions(Table table, Set<Path> missingPartDirs,
     for (Path partPath : missingPartDirs) {
       FileSystem fs = partPath.getFileSystem(conf);
       String partitionName = getPartitionName(fs.makeQualified(tablePath),
-          partPath, partColNames, partitionColToTypeMap);
+          partPath, partColNames, partitionColToTypeMap, conf);
+      if (partitionName == null) {

Review Comment:
   When ` hive.msck.path.validation=skip`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218184051


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   Sure, i will update the same



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218179062


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -100,8 +100,40 @@ POSTHOOK: type: SHOWPARTITIONS
 POSTHOOK: Input: default@tbl_y
 month=12/day=2
 month=12/day=3
-month=12/day=__HIVE_DEFAULT_PARTITION__

Review Comment:
   Same as above



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210390845


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   i don't follow this logic:
   
   if the partitionValue is not a number but type is String, we'll attempt to parse the number in partitionValue - doesn't make any sense



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2206746233


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,41 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if (!NumberUtils.isParsable(partitionValue) && !type.equalsIgnoreCase("string")
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
 
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      if (type.equalsIgnoreCase("tinyint")

Review Comment:
   could this be replaced with switch?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210390845


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   i don't understand this change:
   
   if the partitionValue is not a number but type is String, we'll attempt to parse the number in partitionValue - doesn't make any sense



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218358644


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   > INSERT INTO tbl_x (id, name, month) values(3, 'ccc', 12);
   
   PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)
   
   > SET hive.exec.default.partition.name=ANOTHER_PARTITION;
   > INSERT INTO tbl_x (id, name, day) values(4, 'ddd', 3);
   
   PARTITION(month=ANOTHER_PARTITION,day=3)
   
   What happens to `PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)` data after the MSCK REPAIR? 
   Should it be moved under `PARTITION(month=12,day=ANOTHER_PARTITION)` ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218172518


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) || type == null) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
-
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      switch (type.toLowerCase()) {

Review Comment:
   Yes, it should work. Let me try it out. Thanks for the suggestion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2209438361


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,77 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public enum PartitionDataType {

Review Comment:
   why not use `org.apache.hadoop.hive.metastore.ColumnType`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210375241


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   can non-number be anything else than string type?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2209414605


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,77 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public enum PartitionDataType {
+    TINYINT,
+    SMALLINT,
+    INT,
+    BIGINT,
+    FLOAT,
+    DOUBLE,
+    DECIMAL,
+    STRING,
+    UNKNOWN;
+
+    public static PartitionDataType fromTypeString(String type) {
+      if (type == null || type.isEmpty()){
+        return UNKNOWN;
+      }
+      String base = type.toLowerCase();
+      if (base.startsWith("decimal")){
+        return DECIMAL;
+      }
+      return switch (base) {
+        case "tinyint" -> TINYINT;

Review Comment:
   can you simple use `base.toUpperCase`



##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,77 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public enum PartitionDataType {
+    TINYINT,
+    SMALLINT,
+    INT,
+    BIGINT,
+    FLOAT,
+    DOUBLE,
+    DECIMAL,
+    STRING,
+    UNKNOWN;
+
+    public static PartitionDataType fromTypeString(String type) {
+      if (type == null || type.isEmpty()){
+        return UNKNOWN;
+      }
+      String base = type.toLowerCase();
+      if (base.startsWith("decimal")){
+        return DECIMAL;
+      }
+      return switch (base) {
+        case "tinyint" -> TINYINT;

Review Comment:
   can you simply use `base.toUpperCase`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3139671867

   hi @Aggarwal-Raghav, were your comments resolved? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3077940145

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1527 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1.1% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210333668


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   check for string type seems redundant



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218491696


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   I think for now the user should be moving the data files in case they need it. 
   I think the only reason they will change the default partition property is if they do not need the previous data or want to try something with the new data. In both the cases the old data is not needed. 
   We technically shouldn't be allowing any string partition value other than default partition values for non string partition columns. It can lead to other issues down the line during querying. 
   
   Let me know what you think about it
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2232830843


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   LGTM, @Aggarwal-Raghav, do you see anything major that we should be concerned about?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3078891153

   @vikramahuja1001, please check the comments


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3073136288

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [8 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3072057345

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [7 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2222154896


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   got it, thanks @vikramahuja1001! 
   is https://github.com/apache/hive/pull/5976/files#diff-6465db75ef87970b9c4ccc96661af7ff4aa691c1624eb87688657da70f27d3a6R450 called for PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)?
   @okumin, @Aggarwal-Raghav, could you please review as well since you were involved in the original fix 



##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   got it, thanks @vikramahuja1001! 
   is https://github.com/apache/hive/pull/5976/files#diff-6465db75ef87970b9c4ccc96661af7ff4aa691c1624eb87688657da70f27d3a6R450 called for PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)?
   
   @okumin, @Aggarwal-Raghav, could you please review as well since you were involved in the original fix 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2222154896


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   got it, thanks @vikramahuja1001!
   @okumin, @Aggarwal-Raghav, could you please review as well since you were involved in the original fix 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3125969405

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [20 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3139715804

   > hi @Aggarwal-Raghav, were your comments resolved?
   
   the changes I requested were mostly syntactic sugar i.e string formatting and removing unused imports and using JDK21 style switch expression. They were not functionality specific.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210358193


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) || type == null) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
-
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      switch (type.toLowerCase()) {
+        case ColumnType.TINYINT_TYPE_NAME:
+        case ColumnType.SMALLINT_TYPE_NAME:
+        case ColumnType.INT_TYPE_NAME:
+          return Integer.toString(Integer.parseInt(partitionValue));
+        case ColumnType.BIGINT_TYPE_NAME:
+          return Long.toString(Long.parseLong(partitionValue));
+        case ColumnType.FLOAT_TYPE_NAME:
+          return Float.toString(Float.parseFloat(partitionValue));
+        case ColumnType.DOUBLE_TYPE_NAME:
+          return Double.toString(Double.parseDouble(partitionValue));
+        case ColumnType.DECIMAL_TYPE_NAME:
+          return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+        default:
+          break;
+      }
+    } catch (NumberFormatException e) {

Review Comment:
   there is no point in checking if value is a number and then attempting to parse it 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210333668


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   check for string type seems redundant



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210358193


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) || type == null) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
-
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      switch (type.toLowerCase()) {
+        case ColumnType.TINYINT_TYPE_NAME:
+        case ColumnType.SMALLINT_TYPE_NAME:
+        case ColumnType.INT_TYPE_NAME:
+          return Integer.toString(Integer.parseInt(partitionValue));
+        case ColumnType.BIGINT_TYPE_NAME:
+          return Long.toString(Long.parseLong(partitionValue));
+        case ColumnType.FLOAT_TYPE_NAME:
+          return Float.toString(Float.parseFloat(partitionValue));
+        case ColumnType.DOUBLE_TYPE_NAME:
+          return Double.toString(Double.parseDouble(partitionValue));
+        case ColumnType.DECIMAL_TYPE_NAME:
+          return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+        default:
+          break;
+      }
+    } catch (NumberFormatException e) {

Review Comment:
   there is no point in checking if value is a number and then handling the exception



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218183079


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreChecker.java:
##########
@@ -444,7 +444,12 @@ void findUnknownPartitions(Table table, Set<Path> missingPartDirs,
     for (Path partPath : missingPartDirs) {
       FileSystem fs = partPath.getFileSystem(conf);
       String partitionName = getPartitionName(fs.makeQualified(tablePath),
-          partPath, partColNames, partitionColToTypeMap);
+          partPath, partColNames, partitionColToTypeMap, conf);
+      if (partitionName == null) {
+        // Skip this partition if there is some issue in the partition validation
+        LOG.warn("Skipping partition : " + partPath.getName());
+        continue;
+      }
       LOG.debug("PartitionName: " + partitionName);
 
       if (partitionName != null) {

Review Comment:
   Good idea. I will remove the later null check. It's redundant in our case now



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2230695349


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   It makes sense to me, however, I might be missing some edge-cases. 
   @vikramahuja1001, could you please clarify again what happens in the scenario below 
   >INSERT INTO tbl_x (id, name, month) values(3, 'ccc', 12);
   
   PARTITION(month=12,day=HIVE_DEFAULT_PARTITION)
   
   >SET hive.exec.default.partition.name=ANOTHER_PARTITION;
   >INSERT INTO tbl_x (id, name, day) values(4, 'ddd', 3);
   
   PARTITION(month=ANOTHER_PARTITION,day=3)
   
   >MSCK REPAIR TABLE tbl_x;
   
   Do we log a warning for `PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)` or fail ?



##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   It makes sense to me, however, I might be missing some edge-cases. 
   
   @vikramahuja1001, could you please clarify again what happens in the scenario below 
   >INSERT INTO tbl_x (id, name, month) values(3, 'ccc', 12);
   
   PARTITION(month=12,day=HIVE_DEFAULT_PARTITION)
   
   >SET hive.exec.default.partition.name=ANOTHER_PARTITION;
   >INSERT INTO tbl_x (id, name, day) values(4, 'ddd', 3);
   
   PARTITION(month=ANOTHER_PARTITION,day=3)
   
   >MSCK REPAIR TABLE tbl_x;
   
   Do we log a warning for `PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)` or fail ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2233898228


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,50 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
+    // 1. Handle simple exit cases first.
+    if (type == null) {
       return partitionValue;
     }
-
-    LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
-
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));

Review Comment:
   nit: can we use JDK 21 switch expression here? It looks more clean!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3153088439

   @deniskuzZ , everything has been handled. Let me know if something else needs to be done for this.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218491696


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   I think for now the user should be moving the data files in case they need it. 
   I think the only reason they will change the default partition property is if they do not need the previous data or want to try something with the new data. In both the cases the old data is not needed. 
   
   Let me know what you think about it
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210358193


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)
+            && Objects.equals(partitionValue, MetastoreConf.getVar(conf,
+            MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) || type == null) {
       return partitionValue;
     }
 
     LOG.debug("Converting '" + partitionValue + "' to type: '" + type + "'.");
-
-    if (type.equalsIgnoreCase("tinyint")
-    || type.equalsIgnoreCase("smallint")
-    || type.equalsIgnoreCase("int")){
-      return Integer.toString(Integer.parseInt(partitionValue));
-    } else if (type.equalsIgnoreCase("bigint")){
-      return Long.toString(Long.parseLong(partitionValue));
-    } else if (type.equalsIgnoreCase("float")){
-      return Float.toString(Float.parseFloat(partitionValue));
-    } else if (type.equalsIgnoreCase("double")){
-      return Double.toString(Double.parseDouble(partitionValue));
-    } else if (type.startsWith("decimal")){
-      // Decimal datatypes are stored like decimal(10,10)
-      return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+    try {
+      switch (type.toLowerCase()) {
+        case ColumnType.TINYINT_TYPE_NAME:
+        case ColumnType.SMALLINT_TYPE_NAME:
+        case ColumnType.INT_TYPE_NAME:
+          return Integer.toString(Integer.parseInt(partitionValue));
+        case ColumnType.BIGINT_TYPE_NAME:
+          return Long.toString(Long.parseLong(partitionValue));
+        case ColumnType.FLOAT_TYPE_NAME:
+          return Float.toString(Float.parseFloat(partitionValue));
+        case ColumnType.DOUBLE_TYPE_NAME:
+          return Double.toString(Double.parseDouble(partitionValue));
+        case ColumnType.DECIMAL_TYPE_NAME:
+          return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
+        default:
+          break;
+      }
+    } catch (NumberFormatException e) {

Review Comment:
   there is not point to check if value is a number and then try to parse it 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
       // 1. Handle simple exit cases first.
       if (type == null) {
           return partitionValue;
       }
   
       final String defaultPartitionName = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME);
       if (Objects.equals(partitionValue, defaultPartitionName)) {
           // This is the special partition name for NULL values. It should never be parsed.
           return partitionValue;
       }
   
       // 2. If the type is not numeric, no normalization is needed.
       String normalisedType = ColumnType.getTypeName(type);
       if (!ColumnType.NumericTypes.contains(normalisedType)) {
           return partitionValue;
       }
   
       // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
       try {
           switch (normalisedType) {
               case ColumnType.TINYINT_TYPE_NAME:
               case ColumnType.SMALLINT_TYPE_NAME:
               case ColumnType.INT_TYPE_NAME:
                   return Integer.toString(Integer.parseInt(partitionValue));
               case ColumnType.BIGINT_TYPE_NAME:
                   return Long.toString(Long.parseLong(partitionValue));
               case ColumnType.FLOAT_TYPE_NAME:
                   return Float.toString(Float.parseFloat(partitionValue));
               case ColumnType.DOUBLE_TYPE_NAME:
                   return Double.toString(Double.parseDouble(partitionValue));
               case ColumnType.DECIMAL_TYPE_NAME:
                   // Removes trailing zeros (e.g., 1.200 -> 1.2)
                   return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
               default:
                   // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                   break;
           }
       } catch (NumberFormatException e) {
           // 4. Handle cases where the value cannot be parsed as the expected number type.
           String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
           if ("throw".equals(validationMode)) {
               LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                       "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
               throw e;
           } else if ("skip".equals(validationMode)) {
               LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
               return null; // Signals the caller to skip this partition.
           }
       }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
         return partitionValue;
     }
   
     final String defaultPartitionName = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME);
     if (Objects.equals(partitionValue, defaultPartitionName)) {
         // This is the special partition name for NULL values. It should never be parsed.
         return partitionValue;
     }
   
     // 2. If the type is not numeric, no normalization is needed.
     String normalisedType = ColumnType.getTypeName(type);
     if (!ColumnType.NumericTypes.contains(normalisedType)) {
         return partitionValue;
     }
   
     // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
         switch (normalisedType) {
             case ColumnType.TINYINT_TYPE_NAME:
             case ColumnType.SMALLINT_TYPE_NAME:
             case ColumnType.INT_TYPE_NAME:
                 return Integer.toString(Integer.parseInt(partitionValue));
             case ColumnType.BIGINT_TYPE_NAME:
                 return Long.toString(Long.parseLong(partitionValue));
             case ColumnType.FLOAT_TYPE_NAME:
                 return Float.toString(Float.parseFloat(partitionValue));
             case ColumnType.DOUBLE_TYPE_NAME:
                 return Double.toString(Double.parseDouble(partitionValue));
             case ColumnType.DECIMAL_TYPE_NAME:
                 // Removes trailing zeros (e.g., 1.200 -> 1.2)
                 return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
             default:
                 // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                 break;
         }
     } catch (NumberFormatException e) {
         // 4. Handle cases where the value cannot be parsed as the expected number type.
         String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
         if ("throw".equals(validationMode)) {
             LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                     "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
             throw e;
         } else if ("skip".equals(validationMode)) {
             LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
             return null; // Signals the caller to skip this partition.
         }
     }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
         return partitionValue;
     }
   
     if (Objects.equals(partitionValue, MetastoreConf.getVar(conf, MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
         // This is the special partition name for NULL values. It should never be parsed.
         return partitionValue;
     }
   
     // 2. If the type is not numeric, no normalization is needed.
     String colType = ColumnType.getTypeName(type);
     if (!ColumnType.numericTypes.contains(colType)) {
         return partitionValue;
     }
   
     // 3. At this point, we have a numeric type that needs normalization.
   LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
         switch (colType) {
             case ColumnType.TINYINT_TYPE_NAME:
             case ColumnType.SMALLINT_TYPE_NAME:
             case ColumnType.INT_TYPE_NAME:
                 return Integer.toString(Integer.parseInt(partitionValue));
             case ColumnType.BIGINT_TYPE_NAME:
                 return Long.toString(Long.parseLong(partitionValue));
             case ColumnType.FLOAT_TYPE_NAME:
                 return Float.toString(Float.parseFloat(partitionValue));
             case ColumnType.DOUBLE_TYPE_NAME:
                 return Double.toString(Double.parseDouble(partitionValue));
             case ColumnType.DECIMAL_TYPE_NAME:
                 // Removes trailing zeros (e.g., 1.200 -> 1.2)
                 return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
             default:
                 // This case should not be reachable due to the NUMERIC_TYPES check, but acts as a safeguard.
                 break;
         }
     } catch (NumberFormatException e) {
         // 4. Handle cases where the value cannot be parsed as the expected number type.
         String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
   
         if ("throw".equals(validationMode)) {
             LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. " +
                     "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
             throw e;
         } else if ("skip".equals(validationMode)) {
             LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
             return null; // Signals the caller to skip this partition.
         }
     }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2210484236


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -1636,27 +1638,42 @@ public static String getPartitionName(Path tablePath, Path partitionPath, Set<St
     return result;
   }
 
-  public static String getNormalisedPartitionValue(String partitionValue, String type) {
-
-    if (!NumberUtils.isParsable(partitionValue)) {
+  public static String getNormalisedPartitionValue(String partitionValue, String type,
+      Configuration conf) {
+    if ((!NumberUtils.isParsable(partitionValue) && !Objects.equals(type, ColumnType.STRING_TYPE_NAME)

Review Comment:
   I see it like
   ````
   public static String getNormalisedPartitionValue(String partitionValue, String type, Configuration conf) {
     // 1. Handle simple exit cases first.
     if (type == null) {
       return partitionValue;
     }
     if (Objects.equals(partitionValue, MetastoreConf.getVar(conf,
           MetastoreConf.ConfVars.DEFAULTPARTITIONNAME))) {
       // This is the special partition name for NULL values. It should never be parsed.
       return partitionValue;
     }
     // 2. If the type is not numeric, no normalization is needed.
     String colType = ColumnType.getTypeName(type);
     if (!ColumnType.NumericTypes.contains(colType)) {
       return partitionValue;
     }
     // 3. At this point, we have a numeric type that needs normalization.
     LOG.debug("Normalizing partition value '{}' for type '{}'.", partitionValue, type);
     try {
       switch (colType) {
         case ColumnType.TINYINT_TYPE_NAME:
         case ColumnType.SMALLINT_TYPE_NAME:
         case ColumnType.INT_TYPE_NAME:
           return Integer.toString(Integer.parseInt(partitionValue));
         case ColumnType.BIGINT_TYPE_NAME:
           return Long.toString(Long.parseLong(partitionValue));
         case ColumnType.FLOAT_TYPE_NAME:
           return Float.toString(Float.parseFloat(partitionValue));
         case ColumnType.DOUBLE_TYPE_NAME:
           return Double.toString(Double.parseDouble(partitionValue));
         case ColumnType.DECIMAL_TYPE_NAME:
           return new BigDecimal(partitionValue).stripTrailingZeros().toPlainString();
       }
     } catch (NumberFormatException e) {
       // 4. Handle cases where the value cannot be parsed as the expected number type.
       String validationMode = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.MSCK_PATH_VALIDATION);
       if ("throw".equals(validationMode)) {
         LOG.error("Invalid partition value: Cannot parse '{}' as type '{}'. Failing MSCK. "
             + "Set hive.msck.path.validation=skip to ignore invalid partitions.", partitionValue, type);
         throw e;
       } else if ("skip".equals(validationMode)) {
         LOG.warn("Skipping invalid partition value '{}' for type '{}' due to parsing error.", partitionValue, type);
         // Signals the caller to skip this partition.
         return null;
       }
     }
     return partitionValue;
   }
   ````



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3079191329

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [6 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5976&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5976&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5976&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5976)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ merged PR #5976:
URL: https://github.com/apache/hive/pull/5976


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3124296647

   +0


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on PR #5976:
URL: https://github.com/apache/hive/pull/5976#issuecomment-3127322770

   Thanks for the review @Aggarwal-Raghav 
   
   @deniskuzZ , kindly check again. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218491696


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   I think for now the user should be moving the data files in case they need it. 
   I think the only reason they will change the default partition property is if they do not need the previous data or want to try something with the new data. In both the cases the old data is not needed. 
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2218178318


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   Okay, so this test was buggy to begin with. Since default partition name is already changed to ANOTHER_PARTITION and since these are int partition columns, the only partition other than int type MSCK repair should add is the default partition value which in this case is ANOTHER_PARTITION. 
   So partition value __HIVE_DEFAULT_PARTITION__ shouldn't be added in this case.
   
   This bug is actually induced in HIVE-28654.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2222154896


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   got it, thanks @vikramahuja1001! is https://github.com/apache/hive/pull/5976/files#diff-6465db75ef87970b9c4ccc96661af7ff4aa691c1624eb87688657da70f27d3a6R450 called for PARTITION(month=12,day=__HIVE_DEFAULT_PARTITION__)?
   @okumin, @Aggarwal-Raghav, could you please review as well since you were involved in the original fix 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "vikramahuja1001 (via GitHub)" <gi...@apache.org>.
vikramahuja1001 commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2221564152


##########
ql/src/test/results/clientpositive/llap/msck_repair_9.q.out:
##########
@@ -90,7 +90,7 @@ PREHOOK: Output: default@tbl_y
 POSTHOOK: query: MSCK REPAIR TABLE tbl_y
 POSTHOOK: type: MSCK
 POSTHOOK: Output: default@tbl_y
-Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=12/day=__HIVE_DEFAULT_PARTITION__	tbl_y:month=ANOTHER_PARTITION/day=3
+Partitions not in metastore:	tbl_y:month=12/day=2	tbl_y:month=12/day=3	tbl_y:month=ANOTHER_PARTITION/day=3

Review Comment:
   No regression. It was never working before HIVE-28654. Post HIVE-28654 we are able to add any string partition value to a non string partition column as of now which doesn't make sense. This is a bug induced by HIVE-28654.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org

Re: [PR] HIVE-29089: Fix MSCK Repair table adding invalid partitions for non string partitions columns [hive]

Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #5976:
URL: https://github.com/apache/hive/pull/5976#discussion_r2224675017


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreChecker.java:
##########
@@ -444,34 +444,37 @@ void findUnknownPartitions(Table table, Set<Path> missingPartDirs,
     for (Path partPath : missingPartDirs) {
       FileSystem fs = partPath.getFileSystem(conf);
       String partitionName = getPartitionName(fs.makeQualified(tablePath),
-          partPath, partColNames, partitionColToTypeMap);
+          partPath, partColNames, partitionColToTypeMap, conf);
+      if (partitionName == null) {

Review Comment:
   In which case partitionName is null?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org