PyRSA commented on code in PR #8374:
URL: https://github.com/apache/paimon/pull/8374#discussion_r3504452326
##########
paimon-api/src/main/java/org/apache/paimon/CoreOptions.java:
##########
@@ -2943,6 +2945,13 @@ public List<String> fieldNestedUpdateAggNestedKey(String
fieldName) {
return
Arrays.stream(keyString.split(",")).map(String::trim).collect(Collectors.toList());
}
+ public NestedKeyNullStrategy
fieldNestedUpdateAggNestedKeyNullStrategy(String fieldName) {
+ return options.get(
+ key(FIELDS_PREFIX + "." + fieldName + "." +
NESTED_KEY_NULL_STRATEGY)
+ .enumType(NestedKeyNullStrategy.class)
+ .noDefaultValue());
Review Comment:
> Why not create a default value?
Thanks for the suggestion! I think using a default value also makes sense.
The only issue is that once `MERGE` becomes the default,
`FieldNestedUpdateAgg` can no longer distinguish between an unspecified option
and an explicitly configured `MERGE` in its constructor, so the dependency
validation cannot be performed there anymore.
Instead, I'm thinking of moving the validation to
`FieldNestedUpdateAggFactory.create()`, where the dependency can be validated
before creating the aggregator by checking whether `nested-key-null-strategy`
was explicitly configured. This still provides `fail-fast` validation during
table creation and avoids allowing table options that have no effect.
If this approach sounds reasonable, I'll update the implementation
accordingly.
```java
boolean strategyConfigured =
options.toConfiguration()
.containsKey(...);
checkArgument(
!strategyConfigured || !nestedKey.isEmpty(),
...);
```
Something along these lines.
--
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]
