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

Code before:
	}
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(*table);
	if (wrapper) {
		delete wrapper;
	}
	*table = nullptr;
	return DuckDBSuccess;
}

const char *duckdb_table_description_error(duckdb_table_description table) {
	if (!table) {
		return nullptr;
	}
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table);
	if (wrapper->error.empty()) {
		return nullptr;
	}
	return wrapper->error.c_str();
}

// other methods could be:
// duckdb_column_is_generated(...)
// duckdb_column_get_name(...)
// duckdb_column_get_type(...)

duckdb_state duckdb_column_has_default(duckdb_table_description table_description, idx_t index, bool *out) {
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table_description);
	if (!wrapper || !out) {
		return DuckDBError;
	}

	auto &table = wrapper->description;
	if (index >= table->columns.size()) {
		return DuckDBError;
	}
	auto &column = table->columns[index];
	*out = column.HasDefaultValue();
	return DuckDBSuccess;
}


Review comment (bug):
Are we missing a `nullptr` check here?
```c	
if (!table_description) {
		return DuckDBError; // and set a message?
	}
```

Code after:
	}
	return DuckDBSuccess;
}

void duckdb_table_description_destroy(duckdb_table_description *table) {
	if (!table || !*table) {
		return;
	}
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(*table);
	delete wrapper;
	*table = nullptr;
}

const char *duckdb_table_description_error(duckdb_table_description table) {
	if (!table) {
		return nullptr;
	}
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table);
	if (wrapper->error.empty()) {
		return nullptr;
	}
	return wrapper->error.c_str();
}

duckdb_state duckdb_column_has_default(duckdb_table_description table_description, idx_t index, bool *out) {
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table_description);
	if (!wrapper || !out) {
		if (wrapper) {
			wrapper->error = "Please provide a valid (non-null) 'out' variable";
		}
		return DuckDBError;
	}

	auto &table = wrapper->description;
	if (index >= table->columns.size()) {
		wrapper->error = duckdb::StringUtil::Format("Column index %d is out of range, table only has %d columns", index,
		                                            table->columns.size());
		return DuckDBError;
	}
	auto &column = table->columns[index];
	*out = column.HasDefaultValue();
	return DuckDBSuccess;
}
Repository: duckdb/duckdb
File: src/main/capi/table_description-c.cpp (C++)

Code before:

const char *duckdb_table_description_error(duckdb_table_description table) {
	if (!table) {
		return nullptr;
	}
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table);
	if (wrapper->error.empty()) {
		return nullptr;
	}
	return wrapper->error.c_str();
}

// other methods could be:
// duckdb_column_is_generated(...)
// duckdb_column_get_name(...)
// duckdb_column_get_type(...)

duckdb_state duckdb_column_has_default(duckdb_table_description table_description, idx_t index, bool *out) {
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table_description);
	if (!wrapper || !out) {
		return DuckDBError;
	}

	auto &table = wrapper->description;
	if (index >= table->columns.size()) {
		return DuckDBError;
	}
	auto &column = table->columns[index];
	*out = column.HasDefaultValue();
	return DuckDBSuccess;
}


Review comment (bug):
Also, a side question: do we not have generic errors for these, like 'error: passed nullptr' or 'error: incorrect column index'? Is this something interesting to have a look at?

Code after:
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(*table);
	delete wrapper;
	*table = nullptr;
}

const char *duckdb_table_description_error(duckdb_table_description table) {
	if (!table) {
		return nullptr;
	}
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table);
	if (wrapper->error.empty()) {
		return nullptr;
	}
	return wrapper->error.c_str();
}

duckdb_state duckdb_column_has_default(duckdb_table_description table_description, idx_t index, bool *out) {
	auto wrapper = reinterpret_cast<TableDescriptionWrapper *>(table_description);
	if (!wrapper || !out) {
		if (wrapper) {
			wrapper->error = "Please provide a valid (non-null) 'out' variable";
		}
		return DuckDBError;
	}

	auto &table = wrapper->description;
	if (index >= table->columns.size()) {
		wrapper->error = duckdb::StringUtil::Format("Column index %d is out of range, table only has %d columns", index,
		                                            table->columns.size());
		return DuckDBError;
	}
	auto &column = table->columns[index];
	*out = column.HasDefaultValue();
	return DuckDBSuccess;
}
Repository: duckdb/duckdb
File: tools/pythonpkg/tests/fast/test_expression.py (Python)

Code before:
        b = ColumnExpression("b")

        rel2 = rel.sort(a.asc())
        res = rel2.a.fetchall()
        assert res == [(1,), (2,), (3,), (4,), (5,)]

        # Descending sort order
        rel2 = rel.sort(a.desc())
        res = rel2.a.fetchall()
        assert res == [(5,), (4,), (3,), (2,), (1,)]

        # Nulls first
        rel2 = rel.sort(b.desc().nulls_first())
        res = rel2.b.fetchall()
        assert res == [(None,), ('c',), ('b',), ('a',), ('a',)]

        # Nulls last
        rel2 = rel.sort(b.desc().nulls_last())
        res = rel2.b.fetchall()
        assert res == [('c',), ('b',), ('a',), ('a',), (None,)]

    def test_aggregate(self):
        con = duckdb.connect()
        con.execute("PRAGMA threads=4")
        con.execute("PRAGMA verify_parallelism")

        rel = con.sql("select * from range(1000000) t(a)")
        # Also test multiple reads
        count = FunctionExpression("count", "a").cast("int")
        assert rel.aggregate([count]).execute().fetchone()[0] == 1000000
        assert rel.aggregate([count]).execute().fetchone()[0] == 1000000


Review comment (bug):
Can we add a test where we pass a wrong type to the aggregate function, e.g. `rel.aggregate(42)`, `rel.aggregate([42])`?

Code after:
        b = ColumnExpression("b")

        rel2 = rel.sort(a.asc())
        res = rel2.a.fetchall()
        assert res == [(1,), (2,), (3,), (4,), (5,)]

        # Descending sort order
        rel2 = rel.sort(a.desc())
        res = rel2.a.fetchall()
        assert res == [(5,), (4,), (3,), (2,), (1,)]

        # Nulls first
        rel2 = rel.sort(b.desc().nulls_first())
        res = rel2.b.fetchall()
        assert res == [(None,), ('c',), ('b',), ('a',), ('a',)]

        # Nulls last
        rel2 = rel.sort(b.desc().nulls_last())
        res = rel2.b.fetchall()
        assert res == [('c',), ('b',), ('a',), ('a',), (None,)]

    def test_aggregate(self):
        con = duckdb.connect()
        rel = con.sql("select * from range(1000000) t(a)")
        count = FunctionExpression("count", "a").cast("int")
        assert rel.aggregate([count]).execute().fetchone()[0] == 1000000
        assert rel.aggregate([count]).execute().fetchone()[0] == 1000000

    def test_aggregate_error(self):
        con = duckdb.connect()

        # Not necessarily an error, but even non-aggregates are accepted
        rel = con.sql("select * from values (5) t(a)")
        res = rel.aggregate(["a"]).execute().fetchone()[0]
        assert res == 5

        res = rel.aggregate([5]).execute().fetchone()[0]
        assert res == 5

        # Providing something that can not be converted into an expression is an error:
        with pytest.raises(
            duckdb.InvalidInputException, match='Invalid Input Error: Please provide arguments of type Expression!'
        ):

            class MyClass:
                def __init__(self):
                    pass

            res = rel.aggregate([MyClass()]).fetchone()[0]

        with pytest.raises(
Repository: duckdb/duckdb
File: src/main/relation/join_relation.cpp (C++)

Code before:
      using_columns(std::move(using_columns_p)), join_type(type), join_ref_type(join_ref_type) {
	if (left->context->GetContext() != right->context->GetContext()) {
		throw InvalidInputException("Cannot combine LEFT and RIGHT relations of different connections!");
	}
	TryBindRelation(columns);
}

unique_ptr<QueryNode> JoinRelation::GetQueryNode() {
	auto result = make_uniq<SelectNode>();
	result->select_list.push_back(make_uniq<StarExpression>());
	result->from_table = GetTableRef();
	return std::move(result);
}

namespace {

class BindingDeduplicator {
public:
	BindingDeduplicator() {
	}

public:
	unique_ptr<TableRef> DeduplicateBindings(unique_ptr<TableRef> original) {
		auto &ref = *original;
		switch (ref.type) {
		case TableReferenceType::BASE_TABLE:
		case TableReferenceType::EMPTY_FROM:
		case TableReferenceType::SHOW_REF:
		case TableReferenceType::COLUMN_DATA:
		case TableReferenceType::DELIM_GET:
		case TableReferenceType::SUBQUERY:
		case TableReferenceType::EXPRESSION_LIST: {
			auto &alias = ref.alias;
			if (alias.empty() && ref.type == TableReferenceType::BASE_TABLE) {
				auto &base_table = ref.Cast<BaseTableRef>();
				alias = base_table.table_name;
			}
			if (!alias.empty()) {
				AddBinding(alias);
			}
			return std::move(original);
		}
		case TableReferenceType::JOIN: {
			auto &j_ref = ref.Cast<JoinRef>();
			j_ref.left = DeduplicateBindings(std::move(j_ref.left));
			j_ref.right = DeduplicateBindings(std::move(j_ref.right));
			if (j_ref.condition) {
				ReplaceQualificationRecursive(j_ref.condition);
			}
			return std::move(original);
		}

Review comment (refactor):
In order to simplify this PR, can we maybe remove this and get the simpler fixes in first? Then we can revisit this in the future.

Code after:
	TryBindRelation(columns);
}

unique_ptr<QueryNode> JoinRelation::GetQueryNode() {
	auto result = make_uniq<SelectNode>();
	result->select_list.push_back(make_uniq<StarExpression>());
	result->from_table = GetTableRef();
	return std::move(result);
}

unique_ptr<TableRef> JoinRelation::GetTableRef() {
	auto join_ref = make_uniq<JoinRef>(join_ref_type);
	join_ref->left = left->GetTableRef();
	join_ref->right = right->GetTableRef();
	if (condition) {
		join_ref->condition = condition->Copy();
	}
	join_ref->using_columns = using_columns;
	join_ref->type = join_type;
	join_ref->delim_flipped = delim_flipped;
	for (auto &col : duplicate_eliminated_columns) {
		join_ref->duplicate_eliminated_columns.emplace_back(col->Copy());
	}
	return std::move(join_ref);
}

const vector<ColumnDefinition> &JoinRelation::Columns() {
	return this->columns;
}

string JoinRelation::ToString(idx_t depth) {
	string str = RenderWhitespace(depth);
	str += "Join " + EnumUtil::ToString(join_ref_type) + " " + EnumUtil::ToString(join_type);
	if (condition) {
		str += " " + condition->GetName();
	}

	return str + "\n" + left->ToString(depth + 1) + "\n" + right->ToString(depth + 1);
}

} // namespace duckdb
Repository: duckdb/duckdb
File: src/parser/expression/columnref_expression.cpp (C++)

Code before:
}

ColumnRefExpression::ColumnRefExpression(string column_name)
    : ColumnRefExpression(vector<string> {std::move(column_name)}) {
}

ColumnRefExpression::ColumnRefExpression(vector<string> column_names_p)
    : ParsedExpression(ExpressionType::COLUMN_REF, ExpressionClass::COLUMN_REF),
      column_names(std::move(column_names_p)) {
#ifdef DEBUG
	for (auto &col_name : column_names) {
		D_ASSERT(!col_name.empty());
	}
#endif
}

bool ColumnRefExpression::IsQualified() const {
	return column_names.size() > 1;
}

const string &ColumnRefExpression::GetColumnName() const {
	D_ASSERT(column_names.size() <= 4);
	return column_names.back();
}

void ColumnRefExpression::ReplaceOrRemoveTableName(const string &replacement) {
	auto table_name_index = GetTableNameIndex();
	D_ASSERT(table_name_index.IsValid());
	D_ASSERT(column_names.size() > table_name_index.GetIndex());
	if (replacement.empty()) {
		// Remove all the qualifiers leading up to the table name
		auto offset = static_cast<int64_t>(table_name_index.GetIndex()) + 1;
		column_names.erase(column_names.begin(), column_names.begin() + offset);
	} else {
		column_names[table_name_index.GetIndex()] = replacement;
	}
}

optional_idx ColumnRefExpression::GetTableNameIndex() const {
	if (column_names.size() < 2) {
		return optional_idx();
	}
	// FIXME: this entire function is problematic in combination with structs
	// column_names.size() could exceed 4
	D_ASSERT(column_names.size() <= 4);
	if (column_names.size() == 4) {
		// catalog.schema.table.column
		// FIXME: this also matches for: schema.table.struct_column.field1
		return 2;
	}
	if (column_names.size() == 3) {

Review comment (suggestion):
Let's revert these changes - if you want we can add a `RemoveQualifications` that is used in the changes below

Code after:
}

ColumnRefExpression::ColumnRefExpression(string column_name)
    : ColumnRefExpression(vector<string> {std::move(column_name)}) {
}

ColumnRefExpression::ColumnRefExpression(vector<string> column_names_p)
    : ParsedExpression(ExpressionType::COLUMN_REF, ExpressionClass::COLUMN_REF),
      column_names(std::move(column_names_p)) {
#ifdef DEBUG
	for (auto &col_name : column_names) {
		D_ASSERT(!col_name.empty());
	}
#endif
}

bool ColumnRefExpression::IsQualified() const {
	return column_names.size() > 1;
}

const string &ColumnRefExpression::GetColumnName() const {
	D_ASSERT(column_names.size() <= 4);
	return column_names.back();
}

const string &ColumnRefExpression::GetTableName() const {
	D_ASSERT(column_names.size() >= 2 && column_names.size() <= 4);
	if (column_names.size() == 4) {
		return column_names[2];
	}
	if (column_names.size() == 3) {
		return column_names[1];
	}
	return column_names[0];
}

string ColumnRefExpression::GetName() const {
	return !alias.empty() ? alias : column_names.back();
}

string ColumnRefExpression::ToString() const {
	string result;
	for (idx_t i = 0; i < column_names.size(); i++) {
		if (i > 0) {
			result += ".";
		}
		result += KeywordHelper::WriteOptionallyQuoted(column_names[i]);
	}
	return result;
}
Repository: duckdb/duckdb
File: src/include/duckdb/common/deque.hpp (C++)

Code before:
	typename original::const_reference front() const { // NOLINT: hiding on purpose
		if (MemorySafety<SAFE>::ENABLED && original::empty()) {
			throw InternalException("'front' called on an empty deque!");
		}
		return get<SAFE>(0);
	}

	typename original::reference back() { // NOLINT: hiding on purpose
		if (MemorySafety<SAFE>::ENABLED && original::empty()) {
			throw InternalException("'back' called on an empty deque!");
		}
		return get<SAFE>(original::size() - 1);
	}

	typename original::const_reference back() const { // NOLINT: hiding on purpose
		if (MemorySafety<SAFE>::ENABLED && original::empty()) {
			throw InternalException("'back' called on an empty deque!");
		}
		return get<SAFE>(original::size() - 1);
	}

	void unsafe_erase_at(idx_t idx) { // NOLINT: not using camelcase on purpose here
		original::erase(original::begin() + static_cast<typename original::iterator::difference_type>(idx));
	}

	void erase_at(idx_t idx) { // NOLINT: not using camelcase on purpose here
		if (MemorySafety<SAFE>::ENABLED && idx >= original::size()) {
			throw InternalException("Can't remove offset %d from deque of size %d", idx, original::size());
		}
		unsafe_erase_at(idx);
	}
};

template <typename T>
using unsafe_deque = deque<T, false>;

} // namespace duckdb


Review comment (suggestion):
This is not an existing method on `deque`
When we make safe wrappers the goal is to be an exact carbon copy of the original, not to add extra methods

Code after:
	typename original::const_reference front() const { // NOLINT: hiding on purpose
		if (MemorySafety<SAFE>::ENABLED && original::empty()) {
			throw InternalException("'front' called on an empty deque!");
		}
		return get<SAFE>(0);
	}

	typename original::reference back() { // NOLINT: hiding on purpose
		if (MemorySafety<SAFE>::ENABLED && original::empty()) {
			throw InternalException("'back' called on an empty deque!");
		}
		return get<SAFE>(original::size() - 1);
	}

	typename original::const_reference back() const { // NOLINT: hiding on purpose
		if (MemorySafety<SAFE>::ENABLED && original::empty()) {
			throw InternalException("'back' called on an empty deque!");
		}
		return get<SAFE>(original::size() - 1);
	}
};

template <typename T>
using unsafe_deque = deque<T, false>;

} // namespace duckdb
Repository: duckdb/duckdb
File: tools/shell/shell.cpp (C++)

Code before:
	}
	import_query += ")";
	auto result = con.Query(import_query);
	if (result->HasError()) {
		if (needCommit) {
			con.Rollback();
		}
		string error = StringUtil::Format("Failed To Import Error: Failed to import from file '%s'\n", file_name);
		PrintDatabaseError(error);
		PrintDatabaseError(result->GetError());
		return false;
	}
	if (needCommit) {
		con.Commit();
	}
	return true;
}

string ShellState::EvaluateSQL(const string &sql) {
	if (!db) {
		OpenDB();
	}
	auto &con = *conn;
	auto result = con.Query(sql);
	if (result->HasError()) {
		return "#ERROR#:" + result->GetError();
	}
	auto is_query = result->properties.return_type == duckdb::StatementReturnType::QUERY_RESULT;
	if (is_query == false) {
		return "#NOT A QUERY#";
	}
	auto &collection = result->Collection();
	if (collection.Count() == 0) {
		return "#NO RESULTS#";
	}
	if (collection.Count() > 1) {
		return "#TOO MANY ROWS#";
	}
	if (collection.ColumnCount() != 1) {
		return "#TOO MANY COLUMNS#";
	}

	auto value = collection.GetRows().GetValue(0, 0);
	if (value.IsNull()) {
		return "#NULL#";
	}

	auto str = value.ToString();
	if (str.empty()) {
		return "#EMPTY#";
	}

Review comment (bug):
There were a few more edge cases for the `.open` use case so I added them to make sure each error could be handled explicitly.

Code after:
	auto result = con.Query(import_query);
	if (result->HasError()) {
		if (needCommit) {
			con.Rollback();
		}
		string error = StringUtil::Format("Failed To Import Error: Failed to import from file '%s'\n", file_name);
		PrintDatabaseError(error);
		PrintDatabaseError(result->GetError());
		return false;
	}
	if (needCommit) {
		con.Commit();
	}
	return true;
}

string ShellState::EvaluateSQL(const string &sql) {
	if (!db) {
		OpenDB();
	}
	auto &con = *conn;
	auto result = con.Query(sql);
	if (result->HasError()) {
		return EVAL_SQL_ERROR;
	}
	auto is_query = result->properties.return_type == duckdb::StatementReturnType::QUERY_RESULT;
	if (is_query == false) {
		return EVAL_SQL_NOT_A_QUERY;
	}
	auto &collection = result->Collection();
	if (collection.Count() == 0) {
		return EVAL_SQL_NO_RESULT;
	}
	if (collection.Count() > 1) {
		return EVAL_SQL_TOO_MANY_ROWS;
	}
	if (collection.ColumnCount() != 1) {
		return EVAL_SQL_TOO_MANY_COLUMNS;
	}

	auto value = collection.GetRows().GetValue(0, 0);
	if (value.IsNull()) {
		return EVAL_SQL_NULL;
	}

	auto str = value.ToString();
	if (str.empty()) {
		return EVAL_SQL_EMPTY;
	}

	return str;
Repository: duckdb/duckdb
File: src/execution/index/art/iterator.cpp (C++)

Code before:
				}
				row_id[ROW_ID_SIZE - 1] = byte;
				ARTKey key(&row_id[0], ROW_ID_SIZE);
				row_ids.insert(key.GetRowId());
				if (byte == NumericLimits<uint8_t>::Maximum()) {
					break;
				}
				byte++;
			}
			break;
		}
		default:
			throw InternalException("Invalid leaf type for index scan.");
		}

		entered_nested_leaf = false;
		has_next = Next();
	} while (has_next);
	return true;
}

void Iterator::FindMinimum(const Node &node) {

	auto current_node = node;

	while (true) {
		D_ASSERT(current_node.HasMetadata());

		// Found the minimum.
		if (current_node.IsAnyLeaf()) {
			last_leaf = current_node;
			return;
		}

		// We are passing a gate node.
		if (current_node.GetGateStatus() == GateStatus::GATE_SET) {
			D_ASSERT(status == GateStatus::GATE_NOT_SET);
			status = GateStatus::GATE_SET;
			entered_nested_leaf = true;
			nested_depth = 0;
		}

		// Traverse the prefix.
		if (current_node.GetType() == NType::PREFIX) {
			Prefix prefix(art, current_node);
			for (idx_t i = 0; i < prefix.data[Prefix::Count(art)]; i++) {
				current_key.Push(prefix.data[i]);
				if (status == GateStatus::GATE_SET) {
					row_id[nested_depth] = prefix.data[i];
					nested_depth++;
					D_ASSERT(nested_depth < Prefix::ROW_ID_SIZE);

Review comment (suggestion):
I prefer to put an explicit "end", to avoid `while(true)`, and an exception, if we never expect to end up behind the loop. WDYT?

```cpp
		while (ref.get().HasMetadata()) {
```

Code after:
				}
				row_id[ROW_ID_SIZE - 1] = byte;
				ARTKey key(&row_id[0], ROW_ID_SIZE);
				row_ids.insert(key.GetRowId());
				if (byte == NumericLimits<uint8_t>::Maximum()) {
					break;
				}
				byte++;
			}
			break;
		}
		default:
			throw InternalException("Invalid leaf type for index scan.");
		}

		entered_nested_leaf = false;
		has_next = Next();
	} while (has_next);
	return true;
}

void Iterator::FindMinimum(const Node &node) {
	reference<const Node> ref(node);

	while (ref.get().HasMetadata()) {
		// Found the minimum.
		if (ref.get().IsAnyLeaf()) {
			last_leaf = ref.get();
			return;
		}

		// We are passing a gate node.
		if (ref.get().GetGateStatus() == GateStatus::GATE_SET) {
			D_ASSERT(status == GateStatus::GATE_NOT_SET);
			status = GateStatus::GATE_SET;
			entered_nested_leaf = true;
			nested_depth = 0;
		}

		// Traverse the prefix.
		if (ref.get().GetType() == NType::PREFIX) {
			Prefix prefix(art, ref.get());
			for (idx_t i = 0; i < prefix.data[Prefix::Count(art)]; i++) {
				current_key.Push(prefix.data[i]);
				if (status == GateStatus::GATE_SET) {
					row_id[nested_depth] = prefix.data[i];
					nested_depth++;
					D_ASSERT(nested_depth < Prefix::ROW_ID_SIZE);
				}
			}
			nodes.emplace(ref.get(), 0);