LuciferYang commented on code in PR #13233:
URL: https://github.com/apache/gravitino/pull/13233#discussion_r4056595184
##########
api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distributions.java:
##########
@@ -36,6 +37,20 @@ public class Distributions {
public static final Distribution NONE =
new DistributionImpl(Strategy.NONE, 0, Expression.EMPTY_EXPRESSION);
+ /**
+ * Returns true if the distribution is the NONE distribution. The comparison
is structural so both
+ * the built-in implementation and DTO representations of NONE match.
+ *
+ * @param distribution The distribution to check; may be null.
+ * @return true if the distribution is not null and represents the NONE
distribution.
+ */
+ public static boolean isNone(@Nullable Distribution distribution) {
+ return distribution != null
Review Comment:
Good point, thanks. Fixed in 01b903aa2: `isNone(null)` now returns `true`,
since a null distribution means none was specified. I checked every call site
to confirm it's safe:
- The `checkArgument(isNone(distribution), "... does not support
distribution")` sites (MySQL/PostgreSQL/ClickHouse) no longer throw spuriously
on a null.
- The `!isNone(...)` sites correctly skip applying a distribution for null
(the Hologres one would previously have NPE'd on `validateDistribution(null)`).
- `TableOperationDispatcher` already normalizes null to `Distributions.NONE`
before it reaches catalogs, so this mainly hardens the helper for direct and
future callers, with no runtime regression.
I also removed two now-redundant `== null ||` guards (`DTOConverters`,
`DeltaTableOperations`), updated the Javadoc to state a null distribution is
treated as NONE, and added a test asserting `isNone(null) == true`.
--
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]
