ramitg254 commented on code in PR #6413:
URL: https://github.com/apache/hive/pull/6413#discussion_r3491389525
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/rewrite/MergeRewriter.java:
##########
@@ -238,20 +239,29 @@ public void
appendWhenMatchedUpdateClause(MergeStatement.UpdateClause updateClau
protected void addValues(Table targetTable, String targetAlias,
Map<String, String> newValues,
List<String> values) {
- UnaryOperator<String> formatter = name -> String.format("%s.%s",
targetAlias,
+ UnaryOperator<String> formatter = name -> String.format("%s.%s",
targetAlias,
HiveUtils.unparseIdentifier(name, conf));
-
+ List<String> valuesToBeAdded = new
ArrayList<>(Collections.nCopies(targetTable.getAllCols().size(), null));
for (FieldSchema fieldSchema : targetTable.getCols()) {
- if (newValues.containsKey(fieldSchema.getName())) {
- String rhsExp = newValues.get(fieldSchema.getName());
- values.add(getRhsExpValue(rhsExp,
formatter.apply(fieldSchema.getName())));
- } else {
- values.add(formatter.apply(fieldSchema.getName()));
- }
+ setColumnValue(targetTable, valuesToBeAdded, newValues, formatter,
fieldSchema.getName(), true);
}
-
- targetTable.getPartCols().forEach(fieldSchema -> values.add(
- formatter.apply(fieldSchema.getName())));
+
+ for (FieldSchema partCol : targetTable.getPartCols()) {
+ setColumnValue(targetTable, valuesToBeAdded, newValues, formatter,
partCol.getName(),
+ targetTable.hasNonNativePartitionSupport());
+ }
+ values.addAll(valuesToBeAdded);
+ }
+
+ protected void setColumnValue(Table targetTable, List<String>
valuesToBeAdded,
+ Map<String, String> newValues, UnaryOperator<String> formatter, String
columnName,
+ boolean applyNewValues) {
+ int index = targetTable.getColumnIndexByName(columnName);
+ String formattedColumn = formatter.apply(columnName);
+ String value = applyNewValues && newValues.containsKey(columnName)
+ ? getRhsExpValue(newValues.get(columnName), formattedColumn)
+ : formattedColumn;
+ valuesToBeAdded.set(index, value);
Review Comment:
This one alone is not sufficient as `CopyOnWriteMergeRewriter.java` also
requires the same thing as `SplitMergeRewriter.java` which can be seen via
failure of the test : `merge_iceberg_copy_on_write_partitioned.q` so we have to
either copy the same implementation to `CopyOnWriteMergeRewriter.java` or make
changes in the base class `MergeRewriter.java`
I think we should go for second one :
https://github.com/apache/hive/pull/6413/changes/afbae55f4d06e48863c44e624f5bc7cba9396b4f
( although it violates single responsibility principle in addValues but I
think that is minor trade-off in comparison to the redundancy we will be
introducing if we create similar static class in
`CopyOnWriteMergeRewriter.java`( which we need to if we go for this approach
as that static class would need to inherit the clause generator class of that
class as well) so the redundant code for the same behaviour will be present in
`CopyOnWriteMergeRewriter.java` and `SplitMergeRewriter.java`)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
