Copilot commented on code in PR #2195: URL: https://github.com/apache/fluss/pull/2195#discussion_r2642099797
########## fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/metrics/FlinkHistogramTest.java: ########## @@ -0,0 +1,216 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.fluss.flink.metrics; + +import org.apache.fluss.metrics.Histogram; + +import org.apache.flink.metrics.HistogramStatistics; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.HashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Test for {@link FlinkHistogram}. */ +class FlinkHistogramTest { + + private FlinkHistogram flinkHistogram; + private TestFlussHistogram testFlussHistogram; + + @BeforeEach + void setUp() { + testFlussHistogram = new TestFlussHistogram(); + flinkHistogram = new FlinkHistogram(testFlussHistogram); + } + + @Test + void testUpdate() { + flinkHistogram.update(100L); + assertThat(testFlussHistogram.getUpdateCount()).isEqualTo(1); + assertThat(testFlussHistogram.getLastUpdateValue()).isEqualTo(100L); + } + + @Test + void testGetCount() { + testFlussHistogram.setCount(5L); + assertThat(flinkHistogram.getCount()).isEqualTo(5L); + } + + @Test + void testGetStatisticsReturnsNonNull() { + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics).isNotNull(); + } + + @Test + void testGetStatisticsGetMin() { + testFlussHistogram.setMin(10L); + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics.getMin()).isEqualTo(10L); + } + + @Test + void testGetStatisticsGetMax() { + testFlussHistogram.setMax(100L); + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics.getMax()).isEqualTo(100L); + } + + @Test + void testGetStatisticsGetMean() { + testFlussHistogram.setMean(50.5); + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics.getMean()).isEqualTo(50.5); + } + + @Test + void testGetStatisticsGetStdDev() { + testFlussHistogram.setStdDev(15.2); + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics.getStdDev()).isEqualTo(15.2); + } + + @Test + void testGetStatisticsGetQuantile() { + testFlussHistogram.setQuantile(0.5, 25.0); + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics.getQuantile(0.5)).isEqualTo(25.0); + } + + @Test + void testGetStatisticsGetValues() { + long[] expectedValues = {1L, 2L, 3L}; + testFlussHistogram.setValues(expectedValues); + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics.getValues()).isEqualTo(expectedValues); + } + + @Test + void testGetStatisticsSize() { + testFlussHistogram.setSize(42); + HistogramStatistics statistics = flinkHistogram.getStatistics(); + assertThat(statistics.size()).isEqualTo(42); + } Review Comment: Consider adding a test case that verifies the behavior when the wrapped Fluss histogram's getStatistics() method returns null. This would help ensure that the FlinkHistogram handles this edge case gracefully and doesn't cause a NullPointerException. ########## fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/metrics/FlinkHistogram.java: ########## @@ -41,10 +41,7 @@ public long getCount() { @Override public HistogramStatistics getStatistics() { - - wrapped.getStatistics(); - - return null; + return new FlinkHistogramStatistics(wrapped.getStatistics()); Review Comment: The method should handle the case where wrapped.getStatistics() returns null. While current implementations of Fluss Histogram always return a non-null HistogramStatistics, the interface doesn't explicitly guarantee this. Without a null check, this could still result in a NullPointerException when constructing FlinkHistogramStatistics if a future implementation returns null. Consider adding a null check and either throwing a descriptive exception or returning a default/empty HistogramStatistics implementation. ```suggestion org.apache.fluss.metrics.HistogramStatistics stats = wrapped.getStatistics(); if (stats == null) { throw new IllegalStateException( "Underlying Fluss Histogram returned null HistogramStatistics"); } return new FlinkHistogramStatistics(stats); ``` -- 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]
