ronantakizawa/codereview-bench huggingface dataset:
Repository: duckdb/duckdb
File: tools/pythonpkg/src/pyrelation.cpp (C++)

Code before:
string DuckDBPyRelation::ToString() {
	AssertRelation();
	if (rendered_result.empty()) {
		idx_t limit_rows = 10000;
		BoxRenderer renderer;
		auto limit = Limit(limit_rows, 0);
		auto res = limit->ExecuteInternal();

		auto context = rel->context.GetContext();
		BoxRendererConfig config;
		config.limit = limit_rows;
		rendered_result = res->ToBox(*context, config);
	}
	return rendered_result;
}

void DuckDBPyRelation::Print() {
	py::print(py::str(ToString()));
}

string DuckDBPyRelation::Explain() {
	AssertRelation();
	auto context = rel->context.GetContext();
	auto relation_statement = unique_ptr_cast<RelationStatement, SQLStatement>(make_unique<RelationStatement>(rel));
	auto prepared_statement = context->Prepare(std::move(relation_statement));
	return prepared_statement->data->plan->ToString();
}

// TODO: RelationType to a python enum
py::str DuckDBPyRelation::Type() {
	AssertRelation();
	return py::str(RelationTypeToString(rel->type));
}

py::list DuckDBPyRelation::Columns() {
	AssertRelation();
	py::list res;
	for (auto &col : rel->Columns()) {
		res.append(col.Name());
	}
	return res;
}

py::list DuckDBPyRelation::ColumnTypes() {
	AssertRelation();
	py::list res;
	for (auto &col : rel->Columns()) {
		res.append(col.Type().ToString());
	}
	return res;
}

Review comment (performance):
Could we perhaps go back to using `rel->Explain()`, but read the actual result of the `Explain`? Then this would also work if we change the explain output (e.g. `PRAGMA explain_output = OPTIMIZED_ONLY`).

Code after:
string DuckDBPyRelation::ToString() {
	AssertRelation();
	if (rendered_result.empty()) {
		idx_t limit_rows = 10000;
		BoxRenderer renderer;
		auto limit = Limit(limit_rows, 0);
		auto res = limit->ExecuteInternal();

		auto context = rel->context.GetContext();
		BoxRendererConfig config;
		config.limit = limit_rows;
		rendered_result = res->ToBox(*context, config);
	}
	return rendered_result;
}

void DuckDBPyRelation::Print() {
	py::print(py::str(ToString()));
}

string DuckDBPyRelation::Explain() {
	AssertRelation();
	auto res = rel->Explain();
	D_ASSERT(res->type == duckdb::QueryResultType::MATERIALIZED_RESULT);
	auto &materialized = (duckdb::MaterializedQueryResult &)*res;
	auto &coll = materialized.Collection();
	string result;
	bool skipped_first = false;
	for (auto &row : coll.Rows()) {
		// Skip the first column because it just contains 'physical plan'
		for (idx_t col_idx = 1; col_idx < coll.ColumnCount(); col_idx++) {
			if (col_idx > 1) {
				result += "\t";
			}
			auto val = row.GetValue(col_idx);
			result += val.IsNull() ? "NULL" : StringUtil::Replace(val.ToString(), string("\0", 1), "\\0");
		}
		result += "\n";
	}
	return result;
}

// TODO: RelationType to a python enum
py::str DuckDBPyRelation::Type() {
	AssertRelation();
	return py::str(RelationTypeToString(rel->type));
}

py::list DuckDBPyRelation::Columns() {
	AssertRelation();
	py::list res;
Repository: duckdb/duckdb
File: src/common/types.cpp (C++)

Code before:
	case LogicalTypeId::ENUM:
		return "ENUM";
	case LogicalTypeId::AGGREGATE_STATE:
		return "AGGREGATE_STATE";
	case LogicalTypeId::USER:
		return "USER";
	case LogicalTypeId::BIT:
		return "BIT";
	}
	return "UNDEFINED";
}

string LogicalType::ToString() const {
	auto alias = GetAlias();
	if (!alias.empty()) {
		return alias;
	}
	switch (id_) {
	case LogicalTypeId::STRUCT: {
		if (!type_info_) {
			return "STRUCT";
		}
		auto &child_types = StructType::GetChildTypes(*this);
		string ret = "STRUCT(";
		for (size_t i = 0; i < child_types.size(); i++) {
			ret += KeywordHelper::WriteOptionallyQuoted(child_types[i].first) + " " +
			       KeywordHelper::WriteOptionallyQuoted(child_types[i].second.ToString());
			if (i < child_types.size() - 1) {
				ret += ", ";
			}
		}
		ret += ")";
		return ret;
	}
	case LogicalTypeId::LIST: {
		if (!type_info_) {
			return "LIST";
		}
		return ListType::GetChildType(*this).ToString() + "[]";
	}
	case LogicalTypeId::MAP: {
		if (!type_info_) {
			return "MAP";
		}
		auto &key_type = MapType::KeyType(*this);
		auto &value_type = MapType::ValueType(*this);
		return "MAP(" + key_type.ToString() + ", " + value_type.ToString() + ")";
	}
	case LogicalTypeId::UNION: {
		if (!type_info_) {
			return "UNION";

Review comment (suggestion):
This should only add quotes to the struct field name, not the actual type, otherwise you get these issues:

```
STRUCT(a "STRUCT(b ""STRUCT(a """"INTEGER"""", b """"VARCHAR"""")"")")	
```

Code after:
	case LogicalTypeId::ENUM:
		return "ENUM";
	case LogicalTypeId::AGGREGATE_STATE:
		return "AGGREGATE_STATE";
	case LogicalTypeId::USER:
		return "USER";
	case LogicalTypeId::BIT:
		return "BIT";
	}
	return "UNDEFINED";
}

string LogicalType::ToString() const {
	auto alias = GetAlias();
	if (!alias.empty()) {
		return alias;
	}
	switch (id_) {
	case LogicalTypeId::STRUCT: {
		if (!type_info_) {
			return "STRUCT";
		}
		auto &child_types = StructType::GetChildTypes(*this);
		string ret = "STRUCT(";
		for (size_t i = 0; i < child_types.size(); i++) {
			ret += KeywordHelper::WriteOptionallyQuoted(child_types[i].first) + " " + child_types[i].second.ToString();
			if (i < child_types.size() - 1) {
				ret += ", ";
			}
		}
		ret += ")";
		return ret;
	}
	case LogicalTypeId::LIST: {
		if (!type_info_) {
			return "LIST";
		}
		return ListType::GetChildType(*this).ToString() + "[]";
	}
	case LogicalTypeId::MAP: {
		if (!type_info_) {
			return "MAP";
		}
		auto &key_type = MapType::KeyType(*this);
		auto &value_type = MapType::ValueType(*this);
		return "MAP(" + key_type.ToString() + ", " + value_type.ToString() + ")";
	}
	case LogicalTypeId::UNION: {
		if (!type_info_) {
			return "UNION";
		}
Repository: duckdb/duckdb
File: tools/pythonpkg/tests/fast/pandas/test_datetime_time.py (Python)

Code before:
        df_out = duckdb.query_df(df_in, "df", "select * from df").df()
        pd.testing.assert_frame_equal(df_out, duckdb_time)

    def test_time_timezone_positive_extreme(self, duckdb_cursor):
        duckdb_time = duckdb.query("SELECT make_time(12, 01, 1.000) AS '0'").df()
        # time is 20 hours ahead of UTC
        offset = datetime.timedelta(hours=20)
        timezone = datetime.timezone(offset)
        data = [datetime.time(hour=8, minute=1, second=1, tzinfo=timezone)]
        df_in = pd.DataFrame(
            {'0': pd.Series(data=data, dtype='object')}
        )
        df_out = duckdb.query_df(df_in, "df", "select * from df").df()
        pd.testing.assert_frame_equal(df_out, duckdb_time)

    def test_pandas_datetime_overflow(self):
        duckdb_con = duckdb.connect()

        duckdb_con.execute("create table test (date DATE)")
        duckdb_con.execute("INSERT INTO TEST VALUES ('2263-02-28')")

        with pytest.raises(duckdb.ConversionException):
            res = duckdb_con.execute("select * from test").df()

    def test_timezone_datetime(self):
        module = pytest.importorskip("datetime")
        timezone = module.timezone
        datetime = module.datetime

        con = duckdb.connect()

        dt = datetime.now(timezone.utc).replace(microsecond=0)

        original = dt
        stringified = str(dt)

        original_res = con.execute('select ?::TIMESTAMPTZ', [original]).fetchone()
        stringified_res = con.execute('select ?::TIMESTAMPTZ', [stringified]).fetchone()
        assert original_res == stringified_res


Review comment (suggestion):
`datetime` is built into Python, don't need to use importorskip for it

Code after:
        pd.testing.assert_frame_equal(df_out, duckdb_time)

    def test_time_timezone_positive_extreme(self, duckdb_cursor):
        duckdb_time = duckdb.query("SELECT make_time(12, 01, 1.000) AS '0'").df()
        # time is 20 hours ahead of UTC
        offset = timedelta(hours=20)
        tz = timezone(offset)
        data = [time(hour=8, minute=1, second=1, tzinfo=tz)]
        df_in = pd.DataFrame(
            {'0': pd.Series(data=data, dtype='object')}
        )
        df_out = duckdb.query_df(df_in, "df", "select * from df").df()
        pd.testing.assert_frame_equal(df_out, duckdb_time)

    def test_pandas_datetime_overflow(self):
        duckdb_con = duckdb.connect()

        duckdb_con.execute("create table test (date DATE)")
        duckdb_con.execute("INSERT INTO TEST VALUES ('2263-02-28')")

        with pytest.raises(duckdb.ConversionException):
            res = duckdb_con.execute("select * from test").df()

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

        dt = datetime.now(timezone.utc).replace(microsecond=0)

        original = dt
        stringified = str(dt)

        original_res = con.execute('select ?::TIMESTAMPTZ', [original]).fetchone()
        stringified_res = con.execute('select ?::TIMESTAMPTZ', [stringified]).fetchone()
        assert original_res == stringified_res
Repository: duckdb/duckdb
File: tools/pythonpkg/tests/fast/pandas/test_pandas_df_none.py (Python)

Code before:
import pandas as pd
import pytest
import duckdb
import sys
import gc


class TestPandasDFNone(object):
    def test_none_deref(self):
        con = duckdb.connect()
        gc.collect()
        none_refcount = sys.getrefcount(None)
        for _ in range(1000):
            df = con.sql("select NULL::VARCHAR as a").df()

        gc.collect()
        after = sys.getrefcount(None)
        # Verify that we don't deref None when creating a Pandas DataFrame
        assert after >= none_refcount


Review comment (suggestion):
I don't think the refcount of None is something we can reliably assert on (especially since in 3.12+ it's effectively immortal, see https://github.com/python/cpython/pull/19474) - just having the code that can cause the issue is probably enough

Code after:
import pandas as pd
import pytest
import duckdb
import sys
import gc


class TestPandasDFNone(object):
    # This used to decrease the ref count of None
    def test_none_deref(self):
        con = duckdb.connect()
        df = con.sql("select NULL::VARCHAR as a from range(1000000)").df()
Repository: duckdb/duckdb
File: tools/pythonpkg/tests/fast/pandas/test_pandas_df_none.py (Python)

Code before:
import pandas as pd
import pytest
import duckdb
import sys
import gc


class TestPandasDFNone(object):
    # This used to decrease the ref count of None
    def test_none_deref(self):
        con = duckdb.connect()
        gc.collect()
        for _ in range(1000):
            df = con.sql("select NULL::VARCHAR as a").df()
        gc.collect()


Review comment (bug):
Instead of looping in python, we can probably return a million (or more) `range()` result which immediately crashes the interpreter if you want to verify that ;-)
`db.sql('select null::text from range(1000000)').df()`

Code after:
import pandas as pd
import pytest
import duckdb
import sys
import gc


class TestPandasDFNone(object):
    # This used to decrease the ref count of None
    def test_none_deref(self):
        con = duckdb.connect()
        df = con.sql("select NULL::VARCHAR as a from range(1000000)").df()
Repository: duckdb/duckdb
File: src/function/table/system/duckdb_columns.cpp (C++)

Code before:
	}
	const Value ColumnDefault(idx_t col) override {
		auto &column = entry.GetColumn(LogicalIndex(col));
		if (column.Generated()) {
			return Value(column.GeneratedExpression().ToString());
		} else if (column.DefaultValue()) {
			return Value(column.DefaultValue()->ToString());
		}
		return Value();
	}
	bool IsNullable(idx_t col) override {
		return not_null_cols.find(col) == not_null_cols.end();
	}

private:
	TableCatalogEntry &entry;
	std::set<idx_t> not_null_cols;
};

class ViewColumnHelper : public ColumnHelper {
public:
	explicit ViewColumnHelper(ViewCatalogEntry &entry, ClientContext &context) : entry(entry) {
		auto info = entry.GetInfo();
		auto binder = Binder::CreateBinder(context);
		auto &view_info = info->Cast<CreateViewInfo>();
		binder->BindCreateViewInfo(view_info);
		names = view_info.BoundNames();
		types = view_info.BoundTypes();
		D_ASSERT(names.size() == types.size());

		auto &aliases = entry.aliases;
		if (aliases.size() > names.size()) {
			throw InvalidInputException("View contains more aliases than the columns it's wrapping");
		}
		// Apply the aliases to the names
		for (idx_t i = 0; i < aliases.size(); i++) {
			names[i] = aliases[i];
		}
	}

	StandardEntry &Entry() override {
		return entry;
	}
	idx_t NumColumns() override {
		return types.size();
	}
	const string &ColumnName(idx_t col) override {
		return names[col];
	}
	const LogicalType &ColumnType(idx_t col) override {
		return types[col];

Review comment (performance):
`BindCreateViewInfo` can be quite expensive - since it binds a query (which can involve I/O, e.g. if the view contains a reference to a CSV or Parquet file).

Is there any way we can avoid doing that here?

Code after:
	const Value ColumnDefault(idx_t col) override {
		auto &column = entry.GetColumn(LogicalIndex(col));
		if (column.Generated()) {
			return Value(column.GeneratedExpression().ToString());
		} else if (column.DefaultValue()) {
			return Value(column.DefaultValue()->ToString());
		}
		return Value();
	}
	bool IsNullable(idx_t col) override {
		return not_null_cols.find(col) == not_null_cols.end();
	}

private:
	TableCatalogEntry &entry;
	std::set<idx_t> not_null_cols;
};

class ViewColumnHelper : public ColumnHelper {
public:
	explicit ViewColumnHelper(ViewCatalogEntry &entry) : entry(entry) {
	}

	StandardEntry &Entry() override {
		return entry;
	}
	idx_t NumColumns() override {
		return entry.types.size();
	}
	const string &ColumnName(idx_t col) override {
		return entry.aliases[col];
	}
	const LogicalType &ColumnType(idx_t col) override {
		return entry.types[col];
	}
	const Value ColumnDefault(idx_t col) override {
		return Value();
	}
	bool IsNullable(idx_t col) override {
		return true;
	}

private:
	ViewCatalogEntry &entry;
};

unique_ptr<ColumnHelper> ColumnHelper::Create(CatalogEntry &entry) {
	switch (entry.type) {
	case CatalogType::TABLE_ENTRY:
		return make_uniq<TableColumnHelper>(entry.Cast<TableCatalogEntry>());
	case CatalogType::VIEW_ENTRY:
Repository: duckdb/duckdb
File: src/parser/parsed_data/create_index_info.cpp (C++)

Code before:
	result += "(";
	for (idx_t i = 0; i < parsed_expressions.size(); i++) {
		auto &expr = parsed_expressions[i];
		auto copy = expr->Copy();
		if (i > 0) {
			result += ", ";
		}
		// column ref expressions are qualified with the table name
		// we need to remove them to reproduce the original query
		RemoveTableQualificationRecursive(copy, table);
		result += copy->ToString();
	}
	result += ")";
	if (!options.empty()) {
		result += " WITH (";
		idx_t i = 0;
		for (auto &opt : options) {
			result += StringUtil::Format("%s = %s", opt.first, opt.second.ToString());
			if (i > 0) {
				result += ", ";
			}
			i++;
		}
		result += " )";
	}
	// FIXME: optional WHERE ???
	return result;
}

unique_ptr<CreateInfo> CreateIndexInfo::Copy() const {

	auto result = make_uniq<CreateIndexInfo>(*this);
	CopyProperties(*result);

	for (auto &expr : expressions) {
		result->expressions.push_back(expr->Copy());
	}
	for (auto &expr : parsed_expressions) {
		result->parsed_expressions.push_back(expr->Copy());
	}

	return std::move(result);
}

} // namespace duckdb


Review comment (suggestion):
@Mytherin Same question as above, do we care about losing the WHERE clause?
And also, the WHERE clause is not checked at all currently:
```sql
D create table tbl (a integer);
D create index my_index on tbl(a) where (b IS NOT NULL);
```

(b is not a member of `tbl` so this should probably throw instead)

Code after:
	for (idx_t i = 0; i < parsed_expressions.size(); i++) {
		auto &expr = parsed_expressions[i];
		auto copy = expr->Copy();
		if (i > 0) {
			result += ", ";
		}
		// column ref expressions are qualified with the table name
		// we need to remove them to reproduce the original query
		RemoveTableQualificationRecursive(copy, table);
		result += copy->ToString();
	}
	result += ")";
	if (!options.empty()) {
		result += " WITH (";
		idx_t i = 0;
		for (auto &opt : options) {
			result += StringUtil::Format("%s = %s", opt.first, opt.second.ToString());
			if (i > 0) {
				result += ", ";
			}
			i++;
		}
		result += " )";
	}
	return result;
}

unique_ptr<CreateInfo> CreateIndexInfo::Copy() const {

	auto result = make_uniq<CreateIndexInfo>(*this);
	CopyProperties(*result);

	for (auto &expr : expressions) {
		result->expressions.push_back(expr->Copy());
	}
	for (auto &expr : parsed_expressions) {
		result->parsed_expressions.push_back(expr->Copy());
	}

	return std::move(result);
}

} // namespace duckdb
Repository: duckdb/duckdb
File: src/core_functions/scalar/map/map_entries.cpp (C++)

Code before:
#include "duckdb/core_functions/scalar/map_functions.hpp"
#include "duckdb/planner/expression/bound_function_expression.hpp"
#include "duckdb/common/string_util.hpp"
#include "duckdb/parser/expression/bound_expression.hpp"
#include "duckdb/common/types/data_chunk.hpp"
#include "duckdb/common/pair.hpp"
#include "duckdb/function/scalar/nested_functions.hpp"

namespace duckdb {

// Reverse of map_from_entries
static void MapEntriesFunction(DataChunk &args, ExpressionState &state, Vector &result) {
	auto count = args.size();

	auto &map = args.data[0];
	if (map.GetType().id() == LogicalTypeId::SQLNULL) {
		// Input is a constant NULL
		auto &validity = FlatVector::Validity(result);
		validity.SetInvalid(0);
		result.SetVectorType(VectorType::CONSTANT_VECTOR);
		return;
	}

	MapUtil::ReinterpretMap(result, map, count);

	if (args.AllConstant()) {
		result.SetVectorType(VectorType::CONSTANT_VECTOR);
	}
	result.Verify(count);
}

static LogicalType CreateReturnType(const LogicalType &map) {
	auto &key_type = MapType::KeyType(map);
	auto &value_type = MapType::ValueType(map);

	child_list_t<LogicalType> child_types;
	child_types.push_back(make_pair("key", key_type));
	child_types.push_back(make_pair("value", value_type));

	auto row_type = LogicalType::STRUCT(child_types);
	return LogicalType::LIST(row_type);
}


Review comment (suggestion):
Probably we won't need this anymore either after we have `DEFAULT_NULL_HANDLING`

Code after:
#include "duckdb/core_functions/scalar/map_functions.hpp"
#include "duckdb/planner/expression/bound_function_expression.hpp"
#include "duckdb/common/string_util.hpp"
#include "duckdb/parser/expression/bound_expression.hpp"
#include "duckdb/common/types/data_chunk.hpp"
#include "duckdb/common/pair.hpp"
#include "duckdb/function/scalar/nested_functions.hpp"

namespace duckdb {

// Reverse of map_from_entries
static void MapEntriesFunction(DataChunk &args, ExpressionState &state, Vector &result) {
	auto count = args.size();

	auto &map = args.data[0];
	if (map.GetType().id() == LogicalTypeId::SQLNULL) {
		// Input is a constant NULL
		result.SetVectorType(VectorType::CONSTANT_VECTOR);
		ConstantVector::SetNull(result, true);
		return;
	}

	MapUtil::ReinterpretMap(result, map, count);

	if (args.AllConstant()) {
		result.SetVectorType(VectorType::CONSTANT_VECTOR);
	}
	result.Verify(count);
}

static LogicalType CreateReturnType(const LogicalType &map) {
	auto &key_type = MapType::KeyType(map);
	auto &value_type = MapType::ValueType(map);

	child_list_t<LogicalType> child_types;
	child_types.push_back(make_pair("key", key_type));
	child_types.push_back(make_pair("value", value_type));

	auto row_type = LogicalType::STRUCT(child_types);
	return LogicalType::LIST(row_type);
}

static unique_ptr<FunctionData> MapEntriesBind(ClientContext &context, ScalarFunction &bound_function,
Repository: duckdb/duckdb
File: tools/pythonpkg/src/pyconnection.cpp (C++)

Code before:
}

shared_ptr<DuckDBPyConnection> DuckDBPyConnection::Rollback() {
	ExecuteFromString("ROLLBACK");
	return shared_from_this();
}

shared_ptr<DuckDBPyConnection> DuckDBPyConnection::Checkpoint() {
	ExecuteFromString("CHECKPOINT");
	return shared_from_this();
}

Optional<py::list> DuckDBPyConnection::GetDescription() {
	if (!result) {
		return py::none();
	}
	return result->Description();
}

int DuckDBPyConnection::GetRowcount() {
	return -1;
}

void DuckDBPyConnection::Close() {
	if (connection) {
		Checkpoint();
	}
	result = nullptr;
	connection = nullptr;
	database = nullptr;
	temporary_views.clear();
	// https://peps.python.org/pep-0249/#Connection.close
	for (auto &cur : cursors) {
		cur->Close();
	}
	registered_functions.clear();
	cursors.clear();
}

void DuckDBPyConnection::Interrupt() {
	if (!connection) {
		throw ConnectionException("Connection has already been closed");
	}
	connection->Interrupt();
}

void DuckDBPyConnection::InstallExtension(const string &extension, bool force_install) {
	ExtensionHelper::InstallExtension(*connection->context, extension, force_install);
}

void DuckDBPyConnection::LoadExtension(const string &extension) {

Review comment (suggestion):
We internally checkpoint when the connection goes out of scope. Calling checkpoint explicitly here means that the `disable_checkpoint_on_shutdown` method is not respected. 

Code after:
}

shared_ptr<DuckDBPyConnection> DuckDBPyConnection::Rollback() {
	ExecuteFromString("ROLLBACK");
	return shared_from_this();
}

shared_ptr<DuckDBPyConnection> DuckDBPyConnection::Checkpoint() {
	ExecuteFromString("CHECKPOINT");
	return shared_from_this();
}

Optional<py::list> DuckDBPyConnection::GetDescription() {
	if (!result) {
		return py::none();
	}
	return result->Description();
}

int DuckDBPyConnection::GetRowcount() {
	return -1;
}

void DuckDBPyConnection::Close() {
	result = nullptr;
	connection = nullptr;
	database = nullptr;
	temporary_views.clear();
	// https://peps.python.org/pep-0249/#Connection.close
	for (auto &cur : cursors) {
		cur->Close();
	}
	registered_functions.clear();
	cursors.clear();
}

void DuckDBPyConnection::Interrupt() {
	if (!connection) {
		throw ConnectionException("Connection has already been closed");
	}
	connection->Interrupt();
}

void DuckDBPyConnection::InstallExtension(const string &extension, bool force_install) {
	ExtensionHelper::InstallExtension(*connection->context, extension, force_install);
}

void DuckDBPyConnection::LoadExtension(const string &extension) {
	ExtensionHelper::LoadExternalExtension(*connection->context, extension);
}