Repository: duckdb/duckdb
File: src/storage/compression/bitpacking.cpp (C++)

Code before:
#include "duckdb/function/compression/compression.hpp"
#include "duckdb/function/compression_function.hpp"
#include "duckdb/main/config.hpp"
#include "duckdb/storage/buffer_manager.hpp"
#include "duckdb/storage/statistics/numeric_statistics.hpp"
#include "duckdb/storage/table/column_data_checkpointer.hpp"
#include "duckdb/storage/table/column_segment.hpp"
#include "duckdb/common/operator/subtract.hpp"

#include <functional>

namespace duckdb {

// Note that optimizations in scanning only work if this value is equal to STANDARD_VECTOR_SIZE, however we keep them
// separated to prevent the code from break on lower vector sizes
static constexpr const idx_t BITPACKING_WIDTH_GROUP_SIZE = 1024;

struct EmptyBitpackingWriter {
	template <class T>
	static void Operation(T *values, bool *validity, bitpacking_width_t width, T frame_of_reference, idx_t count,
	                      void *data_ptr) {
	}
};

template <class T>
struct BufferMinMax {
public:
	BufferMinMax() : min_set(false), max_set(false) {
	}
	bool min_set;
	bool max_set;
	T max;
	T min;

public:
	bool TryUpdate(T value) {
		if (!min_set || value < min) {
			min_set = true;
			min = value;
		}
		if (!max_set || value > max) {
			max_set = true;
			max = value;
		}
		T ignore;
		return TrySubtractOperator::Operation(max, min, ignore);
	}
	T Minimum() {
		//! If all values are NULL, the minimum is not set
		if (min_set) {
			return min;

Review comment (bug):
I think this does not need to be a separate Struct. 

I think this would be simplified by removeing the `BufferMinMax` struct by having the bitpacking state contain a `T min`, `T max` and `bool min_max_set` property.  Then a `TryUpdateMinMax(T value)` combined with a `ResetMinMax()` that is called from the Flush.

Also it seems a bit cleaner to first do TrySubtractOperator, and then updating the min/max values when this succeeds because now a `TryUpdate` that fails, still sets the failing min/max values.



Code after:
#include "duckdb/function/compression/compression.hpp"
#include "duckdb/function/compression_function.hpp"
#include "duckdb/main/config.hpp"
#include "duckdb/storage/buffer_manager.hpp"
#include "duckdb/storage/statistics/numeric_statistics.hpp"
#include "duckdb/storage/table/column_data_checkpointer.hpp"
#include "duckdb/storage/table/column_segment.hpp"
#include "duckdb/common/operator/subtract.hpp"

#include <functional>

namespace duckdb {

// Note that optimizations in scanning only work if this value is equal to STANDARD_VECTOR_SIZE, however we keep them
// separated to prevent the code from break on lower vector sizes
static constexpr const idx_t BITPACKING_METADATA_GROUP_SIZE = 1024;

struct EmptyBitpackingWriter {
	template <class T>
	static void Operation(T *values, bool *validity, bitpacking_width_t width, T frame_of_reference, idx_t count,
	                      void *data_ptr) {
	}
};

template <class T>
struct BitpackingState {
public:
	BitpackingState() : compression_buffer_idx(0), total_size(0), data_ptr(nullptr) {
		ResetMinMax();
	}

	T compression_buffer[BITPACKING_METADATA_GROUP_SIZE];
	bool compression_buffer_validity[BITPACKING_METADATA_GROUP_SIZE];
	idx_t compression_buffer_idx;
	idx_t total_size;
	void *data_ptr;

	bool min_max_set;
	T minimum;
	T maximum;

public:
	void SubtractFrameOfReference(const T &frame_of_reference) {
		for (idx_t i = 0; i < compression_buffer_idx; i++) {
			compression_buffer[i] -= frame_of_reference;
		}
	}

	void ResetMinMax() {
		min_max_set = false;
		//! We set these to 0, in case all values are NULL, in which case the min and max will never be set.