Enum type support #193

Tishj · 2024-09-19T08:01:44Z

This PR adds support for Postgres's ENUM types.

In Postgres ENUM types can not be ephemeral (exist only for the duration of a query), they have to be registered, and thus we can't just recreate the ENUM type when converting back from DuckDB -> Postgres at the end of execution.

This detail caused the biggest development overhead, we need to vendor and inherit from a templated derived class that contains the guts of the DuckDB ENUM type so we can add Postgres's ENUM member oids to it.

template <class T>
class PGDuckDBEnumTypeInfo : public EnumTypeInfoTemplated<T> {
public:
	PGDuckDBEnumTypeInfo(Vector &values_insert_order_p, idx_t dict_size_p, Vector &oid_vec)
	    : EnumTypeInfoTemplated<T>(values_insert_order_p, dict_size_p), oid_vec(oid_vec) {
	}

public:
	const Vector &
	GetMemberOids() const {
		return oid_vec;
	}

private:
	Vector oid_vec;
};

We save the ENUM member oids into this, as this is what has to be returned in the query result.

…PGDuckDBEnumTypeInfo to inject the postgres enum OID

…f the enum member (not the enum type oid)

…undtrip these back from duckdb -> postgres in the final result

test/regression/sql/enum.sql

include/pgduckdb/duckdb_vendor/enum_type_info_templated.hpp

include/pgduckdb/types/pgduckdb_enum.hpp

src/pgduckdb_types.cpp

test/regression/sql/enum.sql

src/pgduckdb_types.cpp

include/pgduckdb/types/pgduckdb_enum.hpp

'enum_oid' changed

src/pgduckdb_types.cpp

JelteF · 2024-09-20T12:14:06Z

include/pgduckdb/types/pgduckdb_enum.hpp

+using duckdb::LogicalType;
+using duckdb::Vector;
+
+struct PGDuckDBEnum {


Should this be changed to a namespace? Now that this struct doesn't have any fields anymore?

Yea possibly, in our codebase we're not too strict on static-membered-structs vs namespaces, I think when we also need a constexpr value here we can't use a namespace but can use a struct, so I almost always go for using a struct

Tishj · 2024-09-24T07:14:50Z

I'm waiting on #97 so I can add the tests with a > 'sad' etc, that will involve adding support for looking up type catalog entries, which is probably a nice bonus (but might be better as a separate PR)

To further override `EnumTypeInfoTemplated` it needs to be either vendored (bad) or we move it to a header. Relevant `pg_duckdb` PR: <duckdb/pg_duckdb#193>

wuputah · 2024-10-01T15:51:59Z

Regarding the 0.2.0 milestone: while it's not a blocker for 0.1.0 but it would be great for this to land, especially considering it's basically done. Use of ENUMs is certainly not uncommon in Postgres.

JelteF · 2024-10-01T16:33:46Z

include/pgduckdb/types/pgduckdb_enum.hpp

+// To store additional metadata for a type, DuckDB's LogicalType class contains a 'type_info_' field.
+// The type of this is ExtraTypeInfo, ENUMs make use of this in the form of EnumTypeInfo (see
+// duckdb/include/common/extra_type_info.hpp). We derive from this further to store the ENUMs connection with the oids
+// of Postgres' enum members.


This is a super helpful comment, thanks a lot.

JelteF · 2024-10-02T15:10:47Z

src/catalog/pgduckdb_transaction.cpp

@@ -68,6 +73,56 @@ IsIndexScan(const Path *nodePath) {
 	return false;
 }

+optional_ptr<CatalogEntry>


Let's add a little comment about the assumptions of this function (feel free to correct this comment if my assumptions are incorrect)

Suggested change

optional_ptr<CatalogEntry>

/* Looks up the relevant postgres type and caches it in the SchemaItems.types for the next lookup. This is currently only used to lookup enum types. */

optional_ptr<CatalogEntry>

JelteF · 2024-10-02T15:18:51Z

src/catalog/pgduckdb_transaction.cpp

+	dummy_attr.atttypmod = 0;
+	dummy_attr.attndims = 0;


This seems incorrect if e.g. it's an array an of enum. I think it would be good to add a test for that, it doesn't have to actually work but at least it should throw a somewhat clear error and not crash.

Also I think it would be good to check here for now that the type is an enum, because that's really the only kind of type that we're comfortable with handling here.

JelteF · 2024-10-02T15:25:54Z

src/pgduckdb_types.cpp

+		if (en1->enumsortorder < en2->enumsortorder) {
+			return true; // v1 < v2
+		} else if (en1->enumsortorder > en2->enumsortorder) {
+			return false; // v1 > v2
+		} else {
+			return false; // v1 == v2
+		}


Can be simplified like this:

Suggested change

if (en1->enumsortorder < en2->enumsortorder) {

return true; // v1 < v2

} else if (en1->enumsortorder > en2->enumsortorder) {

return false; // v1 > v2

} else {

return false; // v1 == v2

}

return en1->enumsortorder < en2->enumsortorder;

JelteF · 2024-10-03T08:09:31Z

src/types/pgduckdb_enum.cpp

+// This is medium hacky, we create extra space in the Vector that would normally only hold the strings corresponding to
+// the enum's members. In that extra space we store the enum member oids.
+
+// We do this so we can find the OID from the offset (which is what DuckDB stores in a Vector of type ENUM) and return
+// that when converting the result from DuckDB -> Postgres


This is not true anymore right? i.e. you switched back to the first with the vendored in template (until the next duckdb release)

JelteF · 2024-10-03T08:11:13Z

include/pgduckdb/duckdb_vendor/enum_type_info_templated.hpp

+#include "duckdb/common/serializer/deserializer.hpp"
+
+// This is copied directly without changes from 'duckdb/src/common/extra_type_info.cpp'
+// The reason this is copied is to be able to inherit from it.


Suggested change

// The reason this is copied is to be able to inherit from it.

// The reason this is copied is to be able to inherit from it.

// This can be removed once DuckDB 1.2.0 is released, which contains this PR:

// https://github.com/duckdb/duckdb/pull/14038

JelteF · 2024-10-03T08:43:21Z

src/types/pgduckdb_enum.cpp

+// that when converting the result from DuckDB -> Postgres
+
+LogicalType
+PGDuckDBEnum::CreateEnumType(std::vector<HeapTuple> &enum_members) {


I think it makes sense to merge this function ConvertPostgresEnumToDuckEnum. They are so strongly coupled that I don't think it has any benefit to separate them like this. i.e. ConvertPostgresEnumToDuckEnum is the only caller of this function and that this takes a vector of HeapTuples, which should already be sorted. If we'd keep them separate that requirement on already sorted should be documented in this function its comment, but if we merge the functions such a comment is not necessary.

Tishj added 6 commits September 18, 2024 15:55

add postgres -> duckdb conversion for ENUM types, still need the reverse

1e77b66

vendor EnumTypeInfoTemplated (lives in a cpp file..), subclass it as …

22aae7b

…PGDuckDBEnumTypeInfo to inject the postgres enum OID

duckdb -> postgres binding, return the type_oid we saved earlier

bda3e7f

nearly there, the TupleTableSlot needs to be populated with the OID o…

3cdef4e

…f the enum member (not the enum type oid)

fixed the last problem, we can now consume enums from postgres and ro…

5672b48

…undtrip these back from duckdb -> postgres in the final result

add test + make format

95ad243

JelteF requested changes Sep 19, 2024

View reviewed changes

Tishj mentioned this pull request Sep 19, 2024

[Dev] Move EnumTypeInfoTemplated definition into a hpp file duckdb/duckdb#14038

Merged

Mytherin reviewed Sep 19, 2024

View reviewed changes

include/pgduckdb/types/pgduckdb_enum.hpp Outdated Show resolved Hide resolved

Tishj added 8 commits September 19, 2024 21:06

move into pgduckdb_enum

161e52a

add explanation comments

5eab0b5

Merge branch 'main' into enum_type_support

1eb2d28

added guards

d2f1bb0

fix sortorder issue

0c216c0

dont use elog(ERROR, ..) in duckdb execution path

9eed6e6

last mentions of

bb889f9

'enum_oid' changed

move the implementation details to 'pgduckdb_enum.cpp'

bdfe799

JelteF reviewed Sep 20, 2024

View reviewed changes

src/pgduckdb_types.cpp Outdated Show resolved Hide resolved

explain the implementation details

b46c909

JelteF reviewed Sep 20, 2024

View reviewed changes

Tishj added 3 commits September 24, 2024 11:26

Merge branch 'main' into enum_type_support

1ad91e7

add filtering for enums

bd7d698

added an id column for verification purposes

0ef8cc2

JelteF added this to the 0.2.0 milestone Sep 30, 2024

JelteF added the enhancement New feature or request label Sep 30, 2024

JelteF reviewed Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum type support #193

Enum type support #193

Tishj commented Sep 19, 2024

JelteF Sep 20, 2024

Tishj Sep 24, 2024

Tishj commented Sep 24, 2024

wuputah commented Oct 1, 2024

JelteF Oct 1, 2024

JelteF Oct 2, 2024

JelteF Oct 2, 2024

JelteF Oct 2, 2024

JelteF Oct 3, 2024

JelteF Oct 3, 2024

JelteF Oct 3, 2024

	optional_ptr<CatalogEntry>
	/* Looks up the relevant postgres type and caches it in the SchemaItems.types for the next lookup. This is currently only used to lookup enum types. */
	optional_ptr<CatalogEntry>

Enum type support #193

Are you sure you want to change the base?

Enum type support #193

Conversation

Tishj commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tishj commented Sep 24, 2024

wuputah commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment