voonhous commented on code in PR #19144:
URL: https://github.com/apache/hudi/pull/19144#discussion_r3519468818
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaCompatibilityChecker.java:
##########
@@ -224,18 +237,21 @@ private SchemaCompatibilityResult getCompatibility(final
HoodieSchema reader,
final
Deque<LocationInfo> locations) {
log.debug("Checking compatibility of reader {} with writer {}", reader,
writer);
final ReaderWriter pair = new ReaderWriter(reader, writer);
- SchemaCompatibilityResult result = mMemoizeMap.get(pair);
- if (result != null) {
- if (result.getCompatibilityType() ==
SchemaCompatibilityType.RECURSION_IN_PROGRESS) {
- // Break the recursion here.
- // schemas are compatible unless proven incompatible:
- result = SchemaCompatibilityResult.compatible();
- }
- } else {
- // Mark this reader/writer pair as "in progress":
- mMemoizeMap.put(pair, SchemaCompatibilityResult.recursionInProgress());
- result = calculateCompatibility(reader, writer, locations);
- mMemoizeMap.put(pair, result);
+ final SchemaCompatibilityResult memoized =
memoizedCompatibleResults.get(pair);
+ if (memoized != null) {
+ return memoized;
+ }
+ if (!inProgressPairs.add(pair)) {
+ // Break the recursion here.
+ // schemas are compatible unless proven incompatible:
+ return SchemaCompatibilityResult.compatible();
+ }
+ SchemaCompatibilityResult result = calculateCompatibility(reader,
writer, locations);
+ inProgressPairs.remove(pair);
+ if (result.getCompatibilityType() == SchemaCompatibilityType.COMPATIBLE)
{
+ // Incompatible results embed the field path they were found at, so
they are
+ // recomputed per occurrence; memoizing them would report only the
first path.
+ memoizedCompatibleResults.put(pair, result);
Review Comment:
Good catch, confirmed -- reproduced it in a regression test (plain-field
occurrence memoized COMPATIBLE first, union occurrence of the same pair then
skipped the name check). Fixed as you suggested: the memo key now carries
whether names are validated at that location, computed by the same predicate
`checkSchemaNames` uses, so a verdict is only reusable within the same naming
context. Union-first order was already safe since only COMPATIBLE results are
memoized.
--
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]
