kasakrisz commented on code in PR #6413:
URL: https://github.com/apache/hive/pull/6413#discussion_r3543612096
##########
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:
> violates single responsibility principle in addValues but I think that is
minor trade-off in comparison to the redundancy
IMHO if it is only 2-3 lines of simple code then redundancy is better than
violating single responsibility because it encourages developers to add more
and more responsibility in the future and we can end up with one monolith
component which does everything.
To avoid both code redundancy and big monolith components we need a
different approach: there is
* Copy-On-Write - always non-native and supports partition key update
* Split merge - can be native acid (partition key update not allowed) or
non-native (partition key update allowed)
* Merge - always native acid and partition key update not allowed
Extract the value setting logic to its own component (base class +
subclasses). Copy-On-Write is special because it requires an alias for the
values which are changed. Let's pass this formatter function as a constructor
parameter of the new component.
--
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]
