lincoln-lil commented on code in PR #25217:
URL: https://github.com/apache/flink/pull/25217#discussion_r1723494445
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/utils/UpsertKeyUtil.java:
##########
@@ -42,8 +43,17 @@ public class UpsertKeyUtil {
*/
@Nonnull
public static int[] getSmallestKey(@Nullable Set<ImmutableBitSet>
upsertKeys) {
+ return smallestKey(upsertKeys).orElse(new int[0]);
+ }
+
+ /**
+ * Returns the smallest key of given upsert keys wrapped with a java
{@link Optional}. Different
+ * from {@link #getSmallestKey(Set)}, it'll return result with an empty
{@link Optional} if the
+ * input set is null or empty.
+ */
+ public static Optional<int[]> smallestKey(@Nullable Set<ImmutableBitSet>
upsertKeys) {
Review Comment:
Yes, after some hesitation I ended up adding this new method that returns
`Optional` because of two reasons:
1. the original method returns `int[]` declared as nonnull, but how `int[0]`
is serialized and deserialized on execnode may be implementation related, and
then where the operator is finally called still needs to jointly determine `! =
null && .length > 0` to prevent npe, and `Optional#empty` can definitively use
`isPresent` to check null, and execnode explicitly uses nullable upsertKey to
ignore null values.
2. I'd like to avoid some of the ambiguity caused by `int[0]`, although it's
not exactly the same as the recently fixed FLINK-36000, `Optional#empty` vs
`int[0]` issue.
WDYT?
--
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
