xiangfu0 commented on code in PR #19539:
URL: https://github.com/apache/pinot/pull/19539#discussion_r4070950166
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunction.java:
##########
@@ -63,6 +63,26 @@ default Map<String, String> getFunctionConfig() {
return null;
}
+ /// Returns whether the exposed function configuration is null or empty.
This does not imply that other settings,
+ /// such as the partition id normalizer, have their default values.
+ @JsonIgnore
+ default boolean hasEmptyConfig() {
+ Map<String, String> config = getFunctionConfig();
+ return config == null || config.isEmpty();
+ }
+
+ /// Returns whether partition ids computed by this function can be reused
for the non-null `other` function.
+ /// A true result must guarantee identical results for every input value.
False is conservative, not proof that the
+ /// functions differ. Implementations with additional output-affecting state
must account for it in this method.
+ ///
+ /// The default only permits matching functions with empty exposed
configurations, without comparing config contents.
+ /// Configured implementations may override this method to compare their
effective settings.
+ default boolean canReusePartitionIds(PartitionFunction other) {
+ return hasEmptyConfig() && other.hasEmptyConfig() && getClass() ==
other.getClass()
Review Comment:
Addressed in 115e893c3f. The default now takes the config-content comparison
alternative suggested here: an identity fast path, then exact
class/name/partition count/resolved normalizer and config contents, with null
and empty treated equivalently. The contract explicitly requires reflexivity,
symmetry and transitivity. Murmur/Murmur3/FNV compare immutable effective
fields instead, and their exposed configs are defensively copied. StarTree
Custom already compares its effective expression and normalizer; its companion
tests passed against this interface.
One deliberate exception is BoundedColumnValue: it permits self-reuse only.
Independently deserialized 100 KB configs made generic map equality cost about
105 us versus 0.69 us for the old EQ path in the exploratory 32-segment screen.
The final conservative override keeps the 256-segment large-config controls
within about +/-2%. This follows the agreed Apache/StarTree scope; config
equality does not claim to protect undisclosed state in arbitrary
implementations.
Validation for this commit: 78 focused Apache cases and 46 focused StarTree
cases passed. Full PR CI has both integration suites and compatibility checks
passing; Unit Test Set 2 is separately failing in the unchanged
ZookeeperResourceTest /zk/put HTTP-client path. Resolving this thread for the
implemented config-aware alternative.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
