VARINT and numerics type conversion

[dentiny]

Addresses #598

Sorry for the latency, I was thinking to implement our own conversion from internal storage format of VARINT, then I realize postgres already provides a util function.

[JelteF]

Thanks for working on this. I think this is mostly good, but as in the other comment. Let's move the code to src/pg/types.cpp.

[JelteF]

For this conversion I appreciate the simplicity that the string conversion gives us. Especially since VARINTs are fairly rare types and already a pretty expensive anyway. So converting making the conversion to Postgres a bit more expensive than strictly necessary seems acceptable. This does need a PostgresFunctionGuard though, to keep Postgres C code a bit more physically separated from the C++ code let's move this to logic to the src/pg/types.cpp file. (we're steadily moving more code to the src/pg directory, but we're not done with that migration yet)

[dentiny]

This does need a PostgresFunctionGuard though, to keep Postgres C code a bit more physically separated from the C++ code

Make sense! Function guard added.

Just to confirm, I don't think we need type conversion for numerics <-> VARINT, right?
Because the entrypoint is in pg and we have other mapping (double) in duckdb.

let's move this to logic to the src/pg/types.cpp file.

I could understand the motivation (i.e. place pg related code under pg directory for better maintainability, namespace, etc), but I'm not sure which part to place, should I move all functions there?

As you might be aware of, I'm also working on other type conversions; do you think it ok after I finish them? (I promise I will prioritize them). I'm willing to help migration later on :)

[dentiny]

For this conversion I appreciate the simplicity that the string conversion gives us. Especially since VARINTs are fairly rare types and already a pretty expensive anyway

Yeah I implement via pg native function mostly for code simplicity (but seems other types like TIME could be more easily implement, another story). I leave a comment for the motivation, and what we could improve.

[dentiny]

otherwise compilation failure

src/pgduckdb_types.cpp: In function 'Datum pgduckdb::ConvertNumericDatum(const duckdb::Value&)':
include/pgduckdb/pgduckdb_utils.hpp:106:68: error: no matching function for call to '__PostgresFunctionGuard__<Datum (*)(FunctionCallInfo), numeric_in>(const char [11], Datum, Datum, Datum)'
  106 |         pgduckdb::__PostgresFunctionGuard__<decltype(&FUNC), &FUNC>(#FUNC, ##__VA_ARGS__)
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
src/pgduckdb_types.cpp:346:42: note: in expansion of macro 'PostgresFunctionGuard'
  346 |                 const Datum pg_numeric = PostgresFunctionGuard(numeric_in, CStringGetDatum(value_str.c_str()),
      |                                          ^~~~~~~~~~~~~~~~~~~~~
include/pgduckdb/pgduckdb_utils.hpp:83:1: note: candidate: 'template<class Func, Func func, class ... FuncArgs> typename std::invoke_result<Func, FuncArgs ...>::type pgduckdb::__PostgresFunctionGuard__(const char*, FuncArgs ...)'
   83 | __PostgresFunctionGuard__(const char *func_name, FuncArgs... args) {
      | ^~~~~~~~~~~~~~~~~~~~~~~~~

[JelteF]

I think it makes most sense to create a function in pg/types.cpp

Datum
StringToNumeric(char* str) {
	// ...
}

And then have that function call the actual function with a PostgresFunctionGuard and name that actual function StringToNumeric_C. Similarly to how GetBaseDuckColumnType is implemented.

pg_duckdb/src/pg/types.cpp

Lines 35 to 54 in a8425cb

	static Oid
	GetBaseDuckColumnType_C(Oid attribute_type_oid) {
	Oid typoid = attribute_type_oid;
	if (get_typtype(attribute_type_oid) == TYPTYPE_DOMAIN) {
	/* It is a domain type that needs to be reduced to its base type */
	typoid = getBaseType(attribute_type_oid);
	} else if (type_is_array(attribute_type_oid)) {
	Oid eltoid = get_base_element_type(attribute_type_oid);
	if (OidIsValid(eltoid) && get_typtype(eltoid) == TYPTYPE_DOMAIN) {
	/* When the member type of an array is domain, you need to build a base array type */
	typoid = get_array_type(getBaseType(eltoid));
	}
	}
	return typoid;
	}
	Oid
	GetBaseDuckColumnType(Oid attribute_type_oid) {
	return PostgresFunctionGuard(GetBaseDuckColumnType_C, attribute_type_oid);
	}

[dentiny]

Sounds good! Updated.

[dentiny]

Notice fmgr.h is included twice, sort the headers so we don't get confused.