jelly-1203 commented on a change in pull request #18017:
URL: https://github.com/apache/flink/pull/18017#discussion_r769618830
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/MergeTableLikeUtil.java
##########
@@ -426,6 +433,14 @@ private void appendDerivedColumns(
}
}
+ Set<String> physicalFieldNames =
physicalFieldNamesToTypes.keySet();
+ Set<String> metadataFieldNames =
metadataFieldNamesToTypes.keySet();
+ final Set<String> result = new
LinkedHashSet<>(physicalFieldNames);
+ result.retainAll(metadataFieldNames);
+ if (!result.isEmpty()) {
+ throw new ValidationException(
+ "A field name conflict exists between a field
of the regular type and a field of the Metadata type.");
+ }
Review comment:
> hi, @jelly-1203 thanks for the update and analysis, I agree that it
not possible to add unified check at the end. but I still think that the check
here can be improved a bit:
>
> 1. it seems that the check here is not relevant to the new added
ComputingColumn, if you want to check the duplication, it is better to add it
when updating metadataFieldNamesToTypes.
> 2. according to current implementation, regular columns have top
priority(we collect collectPhysicalFieldsTypes at the beginning), so we may
also need to check if there is duplicated name in physicalFieldNamesToTypes
when trying to a metadata column or computed column. If we add such check, the
check in 1 is not necessary any more.
>
> what do you think?
hi, @wenlong88 I think your suggestion is reasonable, I will try to modify 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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
