You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "JsZero (via GitHub)" <gi...@apache.org> on 2025/09/20 15:40:36 UTC
[PR] HIVE-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
JsZero opened a new pull request, #6090:
URL: https://github.com/apache/hive/pull/6090
…nal Iceberg table may unexpectedly delete other partitions
### What changes were proposed in this pull request?
This PR addresses [HIVE-29218](https://issues.apache.org/jira/browse/HIVE-29218), which reports an issue when using LOAD OVERWRITE on multi-level partitioned Iceberg tables with appendFile as the underlying implementation.
The problem was that the overwrite logic could mistakenly delete metadata of partitions that should not be affected. For example, given a table partitioned by pcol1 and pcol2, with existing data in partition pcol1=x/pcol2=y, running LOAD OVERWRITE into another partition such as pcol1=x/pcol2=z would incorrectly remove the metadata for pcol1=x/pcol2=y just because it partially overlaps on pcol1=x.
The PR fix the overwrite handling logic in Iceberg LOAD implementation to ensure that only the target partition is overwritten.
### Why are the changes needed?
The current implementation may inadvertently delete metadata that should be preserved.
### Does this PR introduce _any_ user-facing change?
No. This change only fixes incorrect behavior where unrelated partitions' metadata might be unintentionally deleted during LOAD OVERWRITE. The intended user-facing behavior remains unchanged.
### How was this patch tested?
ut
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2379949083
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -491,13 +491,11 @@ public static Expression generateExpressionFromPartitionSpec(Table table, Map<St
String partColName = entry.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
- Type resultType = partitionField.transform().getResultType(table.schema()
- .findField(partitionField.sourceId()).type());
- Object value = Conversions.fromPartitionString(resultType, entry.getValue());
- TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
- Iterable<?> iterable = () -> Collections.singletonList(value).iterator();
- if (TransformSpec.TransformType.IDENTITY == transformType) {
- Expression boundPredicate = Expressions.in(partitionField.name(), iterable);
+ Transform<?, ?> transform = partitionField.transform();
+ if (transform.isIdentity()) {
+ Type resultType = transform.getResultType(table.schema().findField(partitionField.sourceId()).type());
Review Comment:
since it's identity no need for getResultType
```
final Type partKeyType = table.schema().findField(partitionField.sourceId()).type();
final Object value = Conversions.fromPartitionString(partKeyType, entry.getValue());
````
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "JsZero (via GitHub)" <gi...@apache.org>.
JsZero commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378660277
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
@aturoczy @ayushtkn @deniskuzZ Thanks a lot for the feedback!
@deniskuzZ That makes sense, I’ll update the code soon.
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2379945826
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -491,13 +491,11 @@ public static Expression generateExpressionFromPartitionSpec(Table table, Map<St
String partColName = entry.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
- Type resultType = partitionField.transform().getResultType(table.schema()
- .findField(partitionField.sourceId()).type());
- Object value = Conversions.fromPartitionString(resultType, entry.getValue());
- TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
- Iterable<?> iterable = () -> Collections.singletonList(value).iterator();
- if (TransformSpec.TransformType.IDENTITY == transformType) {
- Expression boundPredicate = Expressions.in(partitionField.name(), iterable);
+ Transform<?, ?> transform = partitionField.transform();
Review Comment:
please inline inside the if. no need for that local var
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3334654506
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090)
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378490856
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
can we rename to var to `partExpr` ?
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
could we rename to var to `partExpr` ?
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378490856
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
could we rename local var to `partitionExpr` and use `IcebergTableUtil.generateExpressionFromPartitionSpec(...)`?
````
if (partitionSpec != null) {
Expression partitionExpr = IcebergTableUtil.generateExpressionFromPartitionSpec(icebergTbl, partitionSpec, true);
delete.deleteFromRowFilter(partitionExpr);
} else {
delete.deleteFromRowFilter(Expressions.alwaysTrue());
}
````
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378657359
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
would be nice to apply small refactor and remove redundant stuff
````
public static Expression generateExpressionFromPartitionSpec(Table table, Map<String, String> partitionSpec,
boolean latestSpecOnly) throws SemanticException {
Map<String, PartitionField> partitionFieldMap = getPartitionFields(table, latestSpecOnly).stream()
.collect(Collectors.toMap(PartitionField::name, Function.identity()));
Expression finalExp = Expressions.alwaysTrue();
for (Map.Entry<String, String> part : partitionSpec.entrySet()) {
String partColName = part.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
if (TransformSpec.TransformType.IDENTITY == transformType) {
final Type partKeyType = table.schema().findField(partitionField.sourceId()).type();
final Object partKeyVal = Conversions.fromPartitionString(partKeyType, part.getValue());
Expression boundPredicate = Expressions.equal(partColName, partKeyVal);
finalExp = Expressions.and(finalExp, boundPredicate);
} else {
throw new SemanticException(
String.format("Partition transforms are not supported here: %s", partColName));
}
} else {
throw new SemanticException(String.format("No partition column by the name: %s", partColName));
}
}
return finalExp;
}
````
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2379949083
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -491,13 +491,11 @@ public static Expression generateExpressionFromPartitionSpec(Table table, Map<St
String partColName = entry.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
- Type resultType = partitionField.transform().getResultType(table.schema()
- .findField(partitionField.sourceId()).type());
- Object value = Conversions.fromPartitionString(resultType, entry.getValue());
- TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
- Iterable<?> iterable = () -> Collections.singletonList(value).iterator();
- if (TransformSpec.TransformType.IDENTITY == transformType) {
- Expression boundPredicate = Expressions.in(partitionField.name(), iterable);
+ Transform<?, ?> transform = partitionField.transform();
+ if (transform.isIdentity()) {
+ Type resultType = transform.getResultType(table.schema().findField(partitionField.sourceId()).type());
Review Comment:
since it's identity no need for getResultType
```
final Type partKeyType = table.schema().findField(partitionField.sourceId()).type();
final Object partKeyVal = Conversions.fromPartitionString(partKeyType, entry.getValue());
````
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ merged PR #6090:
URL: https://github.com/apache/hive/pull/6090
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378657359
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
would be nice to apply small refactor and remove redundant stuff
````
public static Expression generateExpressionFromPartitionSpec(Table table, Map<String, String> partitionSpec,
boolean latestSpecOnly) throws SemanticException {
Map<String, PartitionField> partitionFieldMap = getPartitionFields(table, latestSpecOnly).stream()
.collect(Collectors.toMap(PartitionField::name, Function.identity()));
Expression finalExp = Expressions.alwaysTrue();
for (Map.Entry<String, String> part : partitionSpec.entrySet()) {
String partColName = part.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
if (TransformSpec.TransformType.IDENTITY == transformType) {
final Type partKeyType = table.schema().findField(partitionField.sourceId()).type();
final Object partKeyVal = Conversions.fromPartitionString(partKeyType, part.getValue());
Expression boundPredicate = Expressions.equal(partColName, partKeyVal);
finalExp = Expressions.and(finalExp, boundPredicate);
} else {
throw new SemanticException(
String.format("Partition transforms are not supported here: %s", partColName));
}
} else {
throw new SemanticException(String.format("No partition column by the name: %s", partColName));
}
}
return finalExp;
}
````
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "JsZero (via GitHub)" <gi...@apache.org>.
JsZero commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3335112817
@deniskuzZ Thanks for approving! I’ve added some small refactorings to improve clarity. Does it look clearer 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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2379953922
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -491,13 +491,11 @@ public static Expression generateExpressionFromPartitionSpec(Table table, Map<St
String partColName = entry.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
- Type resultType = partitionField.transform().getResultType(table.schema()
- .findField(partitionField.sourceId()).type());
- Object value = Conversions.fromPartitionString(resultType, entry.getValue());
- TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
- Iterable<?> iterable = () -> Collections.singletonList(value).iterator();
- if (TransformSpec.TransformType.IDENTITY == transformType) {
- Expression boundPredicate = Expressions.in(partitionField.name(), iterable);
+ Transform<?, ?> transform = partitionField.transform();
+ if (transform.isIdentity()) {
+ Type resultType = transform.getResultType(table.schema().findField(partitionField.sourceId()).type());
Review Comment:
please update also the SemanticException messages
- "Partition transforms are not supported here: %s", partColName));
- "No partition column by the name: %s", partColName));
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378490856
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
could we rename local var to `partitionExpr` ?
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378490856
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
could we rename local var to `partExpr` ?
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378490856
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
could we rename local var to `partitionExpr` and use `IcebergTableUtil.generateExpressionFromPartitionSpec(table, partitionSpec, true)`?
````
if (partitionSpec != null) {
Expression partitionExpr = IcebergTableUtil.generateExpressionFromPartitionSpec(icebergTbl, partitionSpec, true);
delete.deleteFromRowFilter(partitionExpr);
} else {
delete.deleteFromRowFilter(Expressions.alwaysTrue());
}
````
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3335978395
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090)
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "aturoczy (via GitHub)" <gi...@apache.org>.
aturoczy commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3328749653
@ayushtkn @deniskuzZ Could you please review 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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "JsZero (via GitHub)" <gi...@apache.org>.
JsZero commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3336435735
> > @deniskuzZ Thanks for approving! I’ve added some small refactorings to improve clarity. Does it look clearer now?
>
> thanks @JsZero! looks good, added few minor comments
The code refactoring has been incorporated. Thanks a lot for the review @deniskuzZ !
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2379638576
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
thanks!
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3335375632
> @deniskuzZ Thanks for approving! I’ve added some small refactorings to improve clarity. Does it look clearer now?
thanks @JsZero! looks good, added few minor 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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2378490856
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
could we rename local var to `partitionExpr` and use IcebergTableUtil.generateExpressionFromPartitionSpec(table, partitionSpec, true)
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -171,11 +172,13 @@ public static void appendFiles(URI fromURI, String format, Table icebergTbl, boo
if (isOverwrite) {
DeleteFiles delete = transaction.newDelete();
if (partitionSpec != null) {
+ Expression andResult = Expressions.alwaysTrue();
Review Comment:
could we rename local var to `partitionExpr` and use `IcebergTableUtil.generateExpressionFromPartitionSpec(table, partitionSpec, true)`?
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2379945826
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -491,13 +491,11 @@ public static Expression generateExpressionFromPartitionSpec(Table table, Map<St
String partColName = entry.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
- Type resultType = partitionField.transform().getResultType(table.schema()
- .findField(partitionField.sourceId()).type());
- Object value = Conversions.fromPartitionString(resultType, entry.getValue());
- TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
- Iterable<?> iterable = () -> Collections.singletonList(value).iterator();
- if (TransformSpec.TransformType.IDENTITY == transformType) {
- Expression boundPredicate = Expressions.in(partitionField.name(), iterable);
+ Transform<?, ?> transform = partitionField.transform();
Review Comment:
please inline inside the if block. no need for the local var
````
if (partitionField.transform().isIdentity()) {
````
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6090:
URL: https://github.com/apache/hive/pull/6090#discussion_r2379945826
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -491,13 +491,11 @@ public static Expression generateExpressionFromPartitionSpec(Table table, Map<St
String partColName = entry.getKey();
if (partitionFieldMap.containsKey(partColName)) {
PartitionField partitionField = partitionFieldMap.get(partColName);
- Type resultType = partitionField.transform().getResultType(table.schema()
- .findField(partitionField.sourceId()).type());
- Object value = Conversions.fromPartitionString(resultType, entry.getValue());
- TransformSpec.TransformType transformType = TransformSpec.fromString(partitionField.transform().toString());
- Iterable<?> iterable = () -> Collections.singletonList(value).iterator();
- if (TransformSpec.TransformType.IDENTITY == transformType) {
- Expression boundPredicate = Expressions.in(partitionField.name(), iterable);
+ Transform<?, ?> transform = partitionField.transform();
Review Comment:
please inline inside the if block. no need for the local var
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3336527190
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090)
--
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-29218: LOAD OVERWRITE PARTITION on muti-level partititoned exter… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6090:
URL: https://github.com/apache/hive/pull/6090#issuecomment-3315098583
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6090&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6090&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6090&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6090)
--
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