wombatu-kun commented on code in PR #19144:
URL: https://github.com/apache/hudi/pull/19144#discussion_r3519151134
##########
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:
Memoizing COMPATIBLE results keyed only on the value pair (reader, writer)
can mask a union-scope name mismatch and flip the verdict to COMPATIBLE.
`checkSchemaNames` only compares fully-qualified names when the pair is
top-level or directly inside a UNION, so a named record/enum/fixed's verdict is
path-dependent. If such a type is first reached at a plain (non-union) field
path the name check is skipped and the pair is memoized COMPATIBLE; a later
occurrence of the same pair inside a union then hits this memo and returns
COMPATIBLE without ever running the union-scope name check, dropping a genuine
NAME_MISMATCH. Reachable from the `checkNaming=true` entry points
(`isSchemaCompatible` / `checkSchemaCompatible`). The old identity-keyed memo
over fresh navigation wrappers never hit across occurrences, so each path
recomputed and this could not happen. Consider folding the name-checking
context into the memo key.
--
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]
