Fix quantile calculation on small data points (#36)

Author: dentiny

CMakeLists.txt

@@ -25,6 +25,7 @@ set(EXTENSION_SOURCES
     src/filesystem_ref_registry.cpp
     src/histogram.cpp
     src/metrics_collector.cpp
+    src/numeric_utils.cpp
     src/observefs_extension.cpp
     src/observability_filesystem.cpp
     src/operation_latency_collector.cpp

Makefile

@@ -9,7 +9,6 @@ include extension-ci-tools/makefiles/duckdb_extension.Makefile
 
 format-all: format
 	find unit/ -iname *.hpp -o -iname *.cpp | xargs clang-format --sort-includes=0 -style=file -i
-	find benchmark/ -iname *.hpp -o -iname *.cpp | xargs clang-format --sort-includes=0 -style=file -i
 	cmake-format -i CMakeLists.txt
 
 test_unit: all

src/include/numeric_utils.hpp

@@ -0,0 +1,8 @@
+#pragma once
+
+namespace duckdb {
+
+// Return whether the given number A is equal to number B, in a tolerable error range.
+bool IsDoubleEqual(double actual, double expected, bool print_if_unequal = false);
+
+} // namespace duckdb

src/numeric_utils.cpp

@@ -0,0 +1,20 @@
+#include "numeric_utils.hpp"
+
+#include <cmath>
+#include <iostream>
+
+namespace duckdb {
+
+namespace {
+constexpr double MAX_ERROR_RANGE = 0.01;
+} // namespace
+
+bool IsDoubleEqual(double actual, double expected, bool print_if_unequal) {
+	const bool is_equal = std::fabs(actual - expected) <= MAX_ERROR_RANGE;
+	if (!is_equal && print_if_unequal) {
+		std::cerr << "Actual value = " << actual << ", expected value = " << expected << std::endl;
+	}
+	return is_equal;
+}
+
+} // namespace duckdb

src/quantilelite.cpp

@@ -1,16 +1,30 @@
 #include "quantilelite.hpp"
 
 #include <algorithm>
+#include <cmath>
 
 namespace duckdb {
 
 float QuantileLite::Quantile(float q) const {
 	if (samples.empty()) {
 		return 0.0;
 	}
-	const size_t k = static_cast<size_t>(q * (samples.size() - 1));
-	std::nth_element(samples.begin(), samples.begin() + k, samples.end());
-	return samples[k];
+	std::sort(samples.begin(), samples.end());
+
+	const size_t n = samples.size();
+	if (n == 1) {
+		return samples[0];
+	}
+
+	const float pos = q * (n - 1);
+	const size_t lower = static_cast<size_t>(std::floor(pos));
+	const size_t upper = static_cast<size_t>(std::ceil(pos));
+	const float frac = pos - lower;
+
+	if (upper == lower) {
+		return samples[lower];
+	}
+	return samples[lower] + frac * (samples[upper] - samples[lower]);
 }
 
 } // namespace duckdb

unit/test_quantile_estimator.cpp

@@ -4,8 +4,9 @@
 #include <algorithm>
 #include <random>
 
-#include "quantile_estimator.hpp"
 #include "duckdb/common/vector.hpp"
+#include "numeric_utils.hpp"
+#include "quantile_estimator.hpp"
 
 using namespace duckdb; // NOLINT
 
@@ -27,6 +28,50 @@ vector<int> GetRandomNumbers(int max_val) {
 }
 } // namespace
 
+TEST_CASE("Insufficient number of data points", "[quantile test]") {
+	QuantileEstimator qe {METRICS_NAME, METRICS_UNIT};
+
+	// --- One data point ---
+	qe.Add(1);
+	REQUIRE(IsDoubleEqual(qe.p50(), 1.0, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p75(), 1.0, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p90(), 1.0, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p95(), 1.0, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p99(), 1.0, /*print_if_unequal=*/true));
+
+	// --- Two data points ---
+	qe.Add(2);
+	REQUIRE(IsDoubleEqual(qe.p50(), 1.5, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p75(), 1.75, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p90(), 1.9, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p95(), 1.95, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p99(), 1.99, /*print_if_unequal=*/true));
+
+	// --- Three data points ---
+	qe.Add(3);
+	REQUIRE(IsDoubleEqual(qe.p50(), 2.0, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p75(), 2.5, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p90(), 2.8, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p95(), 2.9, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p99(), 2.98, /*print_if_unequal=*/true));
+
+	// --- Four data points ---
+	qe.Add(4);
+	REQUIRE(IsDoubleEqual(qe.p50(), 2.5, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p75(), 3.25, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p90(), 3.7, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p95(), 3.85, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p99(), 3.97, /*print_if_unequal=*/true));
+
+	// --- Five data points ---
+	qe.Add(5);
+	REQUIRE(IsDoubleEqual(qe.p50(), 3.0, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p75(), 4.0, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p90(), 4.6, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p95(), 4.8, /*print_if_unequal=*/true));
+	REQUIRE(IsDoubleEqual(qe.p99(), 4.96, /*print_if_unequal=*/true));
+}
+
 TEST_CASE("Small scale quantile test", "[quantile test]") {
 	constexpr size_t NUM_VALUE = 50;
 	constexpr double MAX_TOLERABLE_DIFF = 1.0;