ronantakizawa/codereview-bench huggingface dataset:
Repository: duckdb/duckdb
File: src/main/appender.cpp (C++)

Code before:
	// check that all vectors have the same length before appending
	if (column != 0) {
		throw InvalidInputException("Failed to Flush appender: incomplete append to row!");
	}

	FlushChunk();
	if (collection->Count() == 0) {
		return;
	}
	FlushInternal(*collection);

	collection->Reset();
	column = 0;
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefault() {
	if (!defaults[column]) {
		throw InvalidInputException("Failed to append DEFAULT, this column does not have a DEFAULT value");
	}
	auto &default_expr = *defaults[column];
	if (!default_expr.IsScalar()) {
		throw InvalidInputException("Only columns with simple DEFAULT values are supported");
	}

	auto default_copy = default_expr.Copy();

	auto &type = types[column];
	auto binder = Binder::CreateBinder(*context);
	ConstantBinder default_binder(*binder, *context, "DEFAULT value");
	default_binder.target_type = type;
	auto bound_default = default_binder.Bind(default_copy);

	Value result_value;
	if (!ExpressionExecutor::TryEvaluateScalar(*context, *bound_default, result_value)) {
		throw InvalidInputException("Could not execute the DEFAULT expression of this column, please insert manually");
	}

	Append(result_value);
}

void InternalAppender::FlushInternal(ColumnDataCollection &collection) {
	auto binder = Binder::CreateBinder(context);
	auto bound_constraints = binder->BindConstraints(table);
	table.GetStorage().LocalAppend(table, context, collection, bound_constraints);
}

void BaseAppender::Close() {

Review comment (suggestion):
Looking for an example that triggers this, the ConstantBinder has support for a `COLUMN_REF` so I figured it was better to be save than sorry

Code after:
}

void BaseAppender::AppendDataChunk(DataChunk &chunk) {
	auto chunk_types = chunk.GetTypes();
	if (chunk_types != types) {
		for (idx_t i = 0; i < chunk.ColumnCount(); i++) {
			if (chunk.data[i].GetType() != types[i]) {
				throw InvalidInputException("Type mismatch in Append DataChunk and the types required for appender, "
				                            "expected %s but got %s for column %d",
				                            types[i].ToString(), chunk.data[i].GetType().ToString(), i + 1);
			}
		}
	}
	collection->Append(chunk);
	if (collection->Count() >= FLUSH_COUNT) {
		Flush();
	}
}

void BaseAppender::FlushChunk() {
	if (chunk.size() == 0) {
		return;
	}
	collection->Append(chunk);
	chunk.Reset();
	if (collection->Count() >= FLUSH_COUNT) {
		Flush();
	}
}

void BaseAppender::Flush() {
	// check that all vectors have the same length before appending
	if (column != 0) {
		throw InvalidInputException("Failed to Flush appender: incomplete append to row!");
	}

	FlushChunk();
	if (collection->Count() == 0) {
		return;
	}
	FlushInternal(*collection);

	collection->Reset();
	column = 0;
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefault() {
Repository: duckdb/duckdb
File: src/main/appender.cpp (C++)

Code before:
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefault() {
	if (!defaults[column]) {
		throw InvalidInputException("Failed to append DEFAULT, this column does not have a DEFAULT value");
	}
	auto &default_expr = *defaults[column];
	if (!default_expr.IsScalar()) {
		throw InvalidInputException("Only columns with simple DEFAULT values are supported");
	}

	auto default_copy = default_expr.Copy();

	auto &type = types[column];
	auto binder = Binder::CreateBinder(*context);
	ConstantBinder default_binder(*binder, *context, "DEFAULT value");
	default_binder.target_type = type;
	auto bound_default = default_binder.Bind(default_copy);

	Value result_value;
	if (!ExpressionExecutor::TryEvaluateScalar(*context, *bound_default, result_value)) {
		throw InvalidInputException("Could not execute the DEFAULT expression of this column, please insert manually");
	}

	Append(result_value);
}

void InternalAppender::FlushInternal(ColumnDataCollection &collection) {
	auto binder = Binder::CreateBinder(context);
	auto bound_constraints = binder->BindConstraints(table);
	table.GetStorage().LocalAppend(table, context, collection, bound_constraints);
}

void BaseAppender::Close() {
	if (column == 0 || column == types.size()) {
		Flush();
	}
}

} // namespace duckdb


Review comment (bug):
This is also more of an assertion, we checked `IsScalar` so this should never fail

Code after:
	collection->Append(chunk);
	if (collection->Count() >= FLUSH_COUNT) {
		Flush();
	}
}

void BaseAppender::FlushChunk() {
	if (chunk.size() == 0) {
		return;
	}
	collection->Append(chunk);
	chunk.Reset();
	if (collection->Count() >= FLUSH_COUNT) {
		Flush();
	}
}

void BaseAppender::Flush() {
	// check that all vectors have the same length before appending
	if (column != 0) {
		throw InvalidInputException("Failed to Flush appender: incomplete append to row!");
	}

	FlushChunk();
	if (collection->Count() == 0) {
		return;
	}
	FlushInternal(*collection);

	collection->Reset();
	column = 0;
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefault() {
	auto it = default_values.find(column);
	auto &column_def = description->columns[column];
	if (it == default_values.end()) {
		throw NotImplementedException(
		    "AppendDefault is currently not supported for column \"%s\" because default expression is not foldable.",
		    column_def.Name());
	}
	auto &default_value = it->second;
	Append(default_value);
}

void InternalAppender::FlushInternal(ColumnDataCollection &collection) {
	auto binder = Binder::CreateBinder(context);
Repository: duckdb/duckdb
File: src/include/duckdb/main/appender.hpp (C++)

Code before:
	void AppendValueInternal(T value);
	template <class SRC, class DST>
	void AppendValueInternal(Vector &vector, SRC input);
	template <class SRC, class DST>
	void AppendDecimalValueInternal(Vector &vector, SRC input);

	void AppendRowRecursive() {
		EndRow();
	}

	template <typename T, typename... ARGS>
	void AppendRowRecursive(T value, ARGS... args) {
		Append<T>(value);
		AppendRowRecursive(args...);
	}

	void AppendValue(const Value &value);
};

class Appender : public BaseAppender {
	//! A reference to a database connection that created this appender
	shared_ptr<ClientContext> context;
	//! The table description (including column names)
	unique_ptr<TableDescription> description;
	//! The default expressions
	vector<optional_ptr<const ParsedExpression>> defaults;

public:
	DUCKDB_API Appender(Connection &con, const string &schema_name, const string &table_name);
	DUCKDB_API Appender(Connection &con, const string &table_name);
	DUCKDB_API ~Appender() override;

public:
	void AppendDefault();

protected:
	void FlushInternal(ColumnDataCollection &collection) override;
};

class InternalAppender : public BaseAppender {
	//! The client context
	ClientContext &context;
	//! The internal table entry to append to
	TableCatalogEntry &table;

public:
	DUCKDB_API InternalAppender(ClientContext &context, TableCatalogEntry &table);
	DUCKDB_API ~InternalAppender() override;

protected:
	void FlushInternal(ColumnDataCollection &collection) override;

Review comment (suggestion):
Perhaps we can store the bound expressions here instead?

We should also always be able to have a default value - if there is none the default value is `NULL`.

Code after:
	void AppendValueInternal(Vector &vector, SRC input);
	template <class SRC, class DST>
	void AppendDecimalValueInternal(Vector &vector, SRC input);

	void AppendRowRecursive() {
		EndRow();
	}

	template <typename T, typename... ARGS>
	void AppendRowRecursive(T value, ARGS... args) {
		Append<T>(value);
		AppendRowRecursive(args...);
	}

	void AppendValue(const Value &value);
};

class Appender : public BaseAppender {
	//! A reference to a database connection that created this appender
	shared_ptr<ClientContext> context;
	//! The table description (including column names)
	unique_ptr<TableDescription> description;
	//! The default expressions
	unordered_map<idx_t, Value> default_values;

public:
	DUCKDB_API Appender(Connection &con, const string &schema_name, const string &table_name);
	DUCKDB_API Appender(Connection &con, const string &table_name);
	DUCKDB_API ~Appender() override;

public:
	void AppendDefault();

protected:
	void FlushInternal(ColumnDataCollection &collection) override;
};

class InternalAppender : public BaseAppender {
	//! The client context
	ClientContext &context;
	//! The internal table entry to append to
	TableCatalogEntry &table;

public:
	DUCKDB_API InternalAppender(ClientContext &context, TableCatalogEntry &table);
	DUCKDB_API ~InternalAppender() override;

protected:
	void FlushInternal(ColumnDataCollection &collection) override;
};
Repository: duckdb/duckdb
File: src/main/appender.cpp (C++)

Code before:
	column = 0;
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefault() {
	if (!defaults[column]) {
		throw InvalidInputException("Failed to append DEFAULT, this column does not have a DEFAULT value");
	}
	auto &default_expr = *defaults[column];
	if (!default_expr.IsScalar()) {
		throw InvalidInputException("Only columns with simple DEFAULT values are supported");
	}

	auto default_copy = default_expr.Copy();

	auto &type = types[column];
	auto binder = Binder::CreateBinder(*context);
	ConstantBinder default_binder(*binder, *context, "DEFAULT value");
	default_binder.target_type = type;
	auto bound_default = default_binder.Bind(default_copy);

	Value result_value;
	if (!ExpressionExecutor::TryEvaluateScalar(*context, *bound_default, result_value)) {
		throw InvalidInputException("Could not execute the DEFAULT expression of this column, please insert manually");
	}

	Append(result_value);
}

void InternalAppender::FlushInternal(ColumnDataCollection &collection) {
	auto binder = Binder::CreateBinder(context);
	auto bound_constraints = binder->BindConstraints(table);
	table.GetStorage().LocalAppend(table, context, collection, bound_constraints);
}

void BaseAppender::Close() {
	if (column == 0 || column == types.size()) {
		Flush();
	}
}

} // namespace duckdb


Review comment (performance):
For a performance optimization - if the expression has `IsFoldable()`, we can cache the `result_value`. This should greatly improve performance in the general case (as this can essentially just become `AppendValue`).

Code after:
	}
	collection->Append(chunk);
	if (collection->Count() >= FLUSH_COUNT) {
		Flush();
	}
}

void BaseAppender::FlushChunk() {
	if (chunk.size() == 0) {
		return;
	}
	collection->Append(chunk);
	chunk.Reset();
	if (collection->Count() >= FLUSH_COUNT) {
		Flush();
	}
}

void BaseAppender::Flush() {
	// check that all vectors have the same length before appending
	if (column != 0) {
		throw InvalidInputException("Failed to Flush appender: incomplete append to row!");
	}

	FlushChunk();
	if (collection->Count() == 0) {
		return;
	}
	FlushInternal(*collection);

	collection->Reset();
	column = 0;
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefault() {
	auto it = default_values.find(column);
	auto &column_def = description->columns[column];
	if (it == default_values.end()) {
		throw NotImplementedException(
		    "AppendDefault is currently not supported for column \"%s\" because default expression is not foldable.",
		    column_def.Name());
	}
	auto &default_value = it->second;
	Append(default_value);
}

void InternalAppender::FlushInternal(ColumnDataCollection &collection) {
Repository: duckdb/duckdb
File: src/main/appender.cpp (C++)

Code before:

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefaultToVector(Vector &result, idx_t column, SelectionVector &sel, idx_t count) {
	// TODO: throw if count is > STANDARD_VECTOR_SIZE
	if (column >= types.size()) {
		throw InvalidInputException(
		    "Could not append DEFAULT value of column %d, out of range index, table only has %d columns", column,
		    types.size());
	}
	auto &type = types[column];
	if (result.GetType() != type) {
		throw InvalidInputException("Type mismatch, column has type: %s, while the provided Vector has type %s",
		                            type.ToString(), result.GetType().ToString());
	}

	auto &executor = *expression_executor;

	// This is only used to tell the ExpressionExecutor the size of our result
	DataChunk empty_chunk;
	empty_chunk.SetCardinality(count);
	executor.SetChunk(empty_chunk);

	// FIXME: if we are feeding the Vector to the ExecuteExpression method, we have to first make sure
	// that the validity mask of the Vector is clean
	context->RunFunctionInTransaction([&]() { executor.ExecuteExpression(column, result, sel); });
}

void Appender::AppendDefault() {
	auto &default_expr = bound_defaults[column];
	if (default_expr->type == ExpressionType::VALUE_CONSTANT) {
		// Fast path: NULL or constant-folded
		auto &bound_constant = default_expr->Cast<BoundConstantExpression>();
		Append(bound_constant.value);
		return;
	}

	auto &type = types[column];
	Vector result(type, 1);
	auto &executor = *expression_executor;
	// The executor is initialized with expressions for every column, even though only some are used
	// this makes it so that we can just use the 'column' index

	// FIXME: we could optimize this further by delaying this until Flush is called, then doing it for the entire chunk
	// once
	context->RunFunctionInTransaction([&]() { executor.ExecuteExpression(column, result); });
	Append(result.GetValue(0));
}


Review comment (suggestion):
Since the `result` is arbitrary provided by the user, we should probably make an intermediate result instead and use SetValue GetValue to transfer the data between them (we can't use Reference/Slice because we don't want to disturb the existing data)

Code after:
}

void Appender::AppendDefault() {
	auto it = default_values.find(column);
	auto &column_def = description->columns[column];
	if (it == default_values.end()) {
		throw NotImplementedException(
		    "AppendDefault is currently not supported for column \"%s\" because default expression is not foldable.",
		    column_def.Name());
	}
	auto &default_value = it->second;
	Append(default_value);
}

void InternalAppender::FlushInternal(ColumnDataCollection &collection) {
	auto binder = Binder::CreateBinder(context);
	auto bound_constraints = binder->BindConstraints(table);
	table.GetStorage().LocalAppend(table, context, collection, bound_constraints);
}

void BaseAppender::Close() {
	if (column == 0 || column == types.size()) {
		Flush();
	}
}

} // namespace duckdb
Repository: duckdb/duckdb
File: test/api/capi/test_capi_appender.cpp (C++)

Code before:
#include "capi_tester.hpp"
#include "duckdb.h"

using namespace duckdb;
using namespace std;

namespace {

struct CAPIAppender {
public:
	CAPIAppender(CAPITester &tester, const char *schema, const char *table) {
		auto status = duckdb_appender_create(tester.connection, schema, table, &appender);
		REQUIRE(status == DuckDBSuccess);
	}
	~CAPIAppender() {
		auto status = duckdb_appender_flush(appender);
		REQUIRE(status == DuckDBSuccess);
		duckdb_appender_close(appender);
		duckdb_appender_destroy(&appender);
	}
	operator duckdb_appender() const {
		return appender;
	}
	operator duckdb_appender *() {
		return &appender;
	}

public:
	duckdb_appender appender = nullptr;
};

} // namespace

void AssertDecimalValueMatches(duckdb::unique_ptr<CAPIResult> &result, duckdb_decimal expected) {
	duckdb_decimal actual;

	actual = result->Fetch<duckdb_decimal>(0, 0);
	REQUIRE(actual.scale == expected.scale);
	REQUIRE(actual.width == expected.width);
	REQUIRE(actual.value.lower == expected.value.lower);
	REQUIRE(actual.value.upper == expected.value.upper);

Review comment (suggestion):
`Flush` is not necessary here. `auto status = duckdb_appender_close(appender)` should be sufficient.

Code after:
#include "capi_tester.hpp"
#include "duckdb.h"

using namespace duckdb;
using namespace std;

namespace {

struct CAPIAppender {
public:
	CAPIAppender(CAPITester &tester, const char *schema, const char *table) {
		auto status = duckdb_appender_create(tester.connection, schema, table, &appender);
		REQUIRE(status == DuckDBSuccess);
	}
	~CAPIAppender() {
		auto status = duckdb_appender_close(appender);
		REQUIRE(status == DuckDBSuccess);
		duckdb_appender_destroy(&appender);
	}
	operator duckdb_appender() const {
		return appender;
	}
	operator duckdb_appender *() {
		return &appender;
	}

public:
	duckdb_appender appender = nullptr;
};

} // namespace

void TestAppenderError(duckdb_appender &appender, const string &expected) {
	auto error = duckdb_appender_error(appender);
	REQUIRE(error != nullptr);
	REQUIRE(duckdb::StringUtil::Contains(error, expected));
}

void AssertDecimalValueMatches(duckdb::unique_ptr<CAPIResult> &result, duckdb_decimal expected) {
	duckdb_decimal actual;
Repository: duckdb/duckdb
File: src/main/appender.cpp (C++)

Code before:
	}

	FlushChunk();
	if (collection->Count() == 0) {
		return;
	}
	FlushInternal(*collection);

	collection->Reset();
	column = 0;
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefaultsToVector(Vector &result, idx_t column, SelectionVector &sel, idx_t count) {
#ifdef DEBUG
	for (idx_t i = 0; i < count; i++) {
		auto index = sel.get_index(i);
		if (index > STANDARD_VECTOR_SIZE) {
			// FIXME: should we require the size of the 'result' Vector as well?
			throw InternalException("Provided SelectionVector is invalid, index %d points to %d, which is out of range",
			                        i, index);
		}
	}
#endif
	if (count > STANDARD_VECTOR_SIZE) {
		throw InvalidInputException("Please provide a Vector that is at most STANDARD_VECTOR_SIZE (%d) in size",
		                            STANDARD_VECTOR_SIZE);
	}
	if (column >= types.size()) {
		throw InvalidInputException(
		    "Could not append DEFAULT value of column %d, out of range index, table only has %d columns", column,
		    types.size());
	}
	auto &type = types[column];
	if (result.GetType() != type) {
		throw InvalidInputException("Type mismatch, column has type: %s, while the provided Vector has type %s",
		                            type.ToString(), result.GetType().ToString());
	}
	auto &executor = *expression_executor;

	// Create an empty chunk to tell the ExpressionExecutor how many tuples to execute for
	DataChunk empty_chunk;
	empty_chunk.SetCardinality(count);
	executor.SetChunk(empty_chunk);

	Vector intermediate(type, count);
	context->RunFunctionInTransaction([&]() { executor.ExecuteExpression(column, intermediate); });
	for (idx_t i = 0; i < count; i++) {

Review comment (refactor):
Maybe we can move this into a `Verify` function on the selection vector itself? We can also use it in other places where we construct selection vectors. The vector size might already be sufficiently tested by the `vector.Verify(count)` functions?

Code after:
	if (collection->Count() == 0) {
		return;
	}
	FlushInternal(*collection);

	collection->Reset();
	column = 0;
}

void Appender::FlushInternal(ColumnDataCollection &collection) {
	context->Append(*description, collection);
}

void Appender::AppendDefault() {
	auto it = default_values.find(column);
	auto &column_def = description->columns[column];
	if (it == default_values.end()) {
		throw NotImplementedException(
		    "AppendDefault is currently not supported for column \"%s\" because default expression is not foldable.",
		    column_def.Name());
	}
	auto &default_value = it->second;
	Append(default_value);
}

void InternalAppender::FlushInternal(ColumnDataCollection &collection) {
	auto binder = Binder::CreateBinder(context);
	auto bound_constraints = binder->BindConstraints(table);
	table.GetStorage().LocalAppend(table, context, collection, bound_constraints);
}

void BaseAppender::Close() {
	if (column == 0 || column == types.size()) {
		Flush();
	}
}

} // namespace duckdb
Repository: duckdb/duckdb
File: src/main/appender.cpp (C++)

Code before:
	}
	auto binder = Binder::CreateBinder(*context);

	context->RunFunctionInTransaction([&]() {
		for (idx_t i = 0; i < types.size(); i++) {
			auto &type = types[i];
			auto &expr = defaults[i];

			if (!expr) {
				// Insert NULL
				bound_defaults.push_back(make_uniq<BoundConstantExpression>(Value(type)));
				continue;
			}
			auto default_copy = expr->Copy();
			D_ASSERT(!default_copy->HasParameter());
			ConstantBinder default_binder(*binder, *context, "DEFAULT value");
			default_binder.target_type = type;
			auto bound_default = default_binder.Bind(default_copy);
			Value result_value;
			if (bound_default->IsFoldable() &&
			    ExpressionExecutor::TryEvaluateScalar(*context, *bound_default, result_value)) {
				// Insert the evaluated Value
				bound_defaults.push_back(make_uniq<BoundConstantExpression>(result_value));
			} else {
				// Insert a bound Expression
				bound_defaults.push_back(std::move(bound_default));
			}
		}
	});

	expression_executor = make_uniq<ExpressionExecutor>(*context, bound_defaults);
	InitializeChunk();
	collection = make_uniq<ColumnDataCollection>(allocator, types);
}

Appender::Appender(Connection &con, const string &table_name) : Appender(con, DEFAULT_SCHEMA, table_name) {
}

Appender::~Appender() {
	Destructor();
}

void BaseAppender::InitializeChunk() {
	chunk.Initialize(allocator, types);
}

void BaseAppender::BeginRow() {
}

void BaseAppender::EndRow() {
	// check that all rows have been appended to

Review comment (suggestion):
Same with the bound defaults - this could be a `vector<Value>` instead

Code after:
	auto binder = Binder::CreateBinder(*context);

	context->RunFunctionInTransaction([&]() {
		for (idx_t i = 0; i < types.size(); i++) {
			auto &type = types[i];
			auto &expr = defaults[i];

			if (!expr) {
				// Insert NULL
				default_values[i] = Value(type);
				continue;
			}
			auto default_copy = expr->Copy();
			D_ASSERT(!default_copy->HasParameter());
			ConstantBinder default_binder(*binder, *context, "DEFAULT value");
			default_binder.target_type = type;
			auto bound_default = default_binder.Bind(default_copy);
			Value result_value;
			if (bound_default->IsFoldable() &&
			    ExpressionExecutor::TryEvaluateScalar(*context, *bound_default, result_value)) {
				// Insert the evaluated Value
				default_values[i] = result_value;
			} else {
				// These are not supported currently, we don't add them to the 'default_values' map
			}
		}
	});

	InitializeChunk();
	collection = make_uniq<ColumnDataCollection>(allocator, types);
}

Appender::Appender(Connection &con, const string &table_name) : Appender(con, DEFAULT_SCHEMA, table_name) {
}

Appender::~Appender() {
	Destructor();
}

void BaseAppender::InitializeChunk() {
	chunk.Initialize(allocator, types);
}

void BaseAppender::BeginRow() {
}

void BaseAppender::EndRow() {
	// check that all rows have been appended to
	if (column != chunk.ColumnCount()) {
		throw InvalidInputException("Call to EndRow before all rows have been appended to!");
	}