yihua commented on PR #19610:
URL: https://github.com/apache/hudi/pull/19610#issuecomment-5286167154
The diagnosis looks right to me, and I agree the existing checks don't cover
this shape. But there are already two places in this file that resolve
partition columns, and the new lazy val is unaware of both. I think that leaves
a regression and a duplicate-projection case, and the consolidation opportunity
is with the second one.
**Existing check 1: `recordKeyAttributeToConditionExpression`.** It already
enumerates partition fields (`partitionAndKeyFields ++ partitionPathFields...`)
and already feeds them into `requiredAttributesMap`, but only from the `ON`
condition, and the throw is gated on `rk._1.equals("primaryKey")` so partition
columns stay optional (see the `//allow partition path to be part of the merge
condition but not required` note).
**Existing check 2: `checkSchemaMergeIntoCompatibility`.** It resolves each
partition field with the same helper this PR calls, purely to type-check it,
and then discards the failure:
```scala
} catch {
case _: MergeIntoFieldResolutionException =>
}
```
That swallow is what let this bug survive. It's also where I'd consolidate:
have `checkSchemaMergeIntoCompatibility` consume the new
`partitionFieldsAssociatedExpressions` for its `validateDataTypes` call instead
of re-resolving per field, and drop the try/catch, which becomes dead once
resolution fails earlier. That removes the duplicated resolution and the trap.
I don't think the fix can move wholesale into `validate()`, for what it's
worth: the association is consumed by `requiredAttributesMap` to back-fill the
column, and `checkSchemaMergeIntoCompatibility` returns `Unit`, so putting it
there would reduce this to "reject instead of corrupt" and lose the
assignment-derived case. Keeping it next to
`orderingFieldsAssociatedExpressions` seems right.
Two things worth checking before/while consolidating, both from
concatenating the new lazy val at `requiredAttributesMap` without reconciling
against check 1:
1. **Regression.** `resolveFieldAssociationsBetweenSourceAndTarget` looks at
source output, then assignments. It never consults the merge condition. So `ON
t.id = s.id AND t.dt = s.date_col` with no `dt` assignment resolves today via
check 1 and would now be rejected. The description lists the `ON` condition as
a derivation source, but the helper doesn't read it. Suggest falling back to
the associations check 1 already produced before throwing, and adding a test
for the differently-named-source-column shape.
2. **Duplicate projection.** When a partition column is derivable from an
assignment and also named in the `ON` condition, both paths emit an entry for
the same target attribute. `requiredAttributesMap` does no `distinct`, and the
`missingAttributesMap.flatMap` below adds one `Alias(..., "dt")` per entry, so
the projection can end up with two `dt` columns. Suggest deduping
`requiredAttributesMap` by target attribute.
Minor: the error message is quite long for a thrown exception. The remedy
sentence ("project the partition column(s) [...] in the source query") is the
actionable part; the two failure modes read more like a code comment than
something a user needs at the prompt, and they're already captured in the
javadoc.
--
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]
