support extended list types #282

Reminiscent · 2024-10-14T14:06:40Z

Related to #101

This pull request adds more array types. The changes include:

Extended Array Type Support:

Added support for additional array types including CHAR, SMALLINT, VARCHAR, TIMESTAMP, FLOAT4, FLOAT8, NUMERIC, UUID and JSON.
Implemented support for both single-dimension and two-dimensional arrays for these types.

Improved Type Conversion:

Refactored the type conversion logic to handle the new array types efficiently.
Implemented specialized conversion functions for each supported data type to ensure accurate data representation between PostgreSQL and DuckDB.

Enhanced PostgresArrayAppendState:

Modified to handle edge cases, such as empty arrays and NULL values, more robustly.

Updated Test Suite:

Expanded the test suite to cover the newly supported array types and dimensions.

Code Refactoring:

Introduced a more generic approach to array handling, reducing code duplication and improving maintainability.
Implemented the PostgresOIDMapping structure to streamline type-specific operations.

JelteF · 2024-10-14T15:33:08Z

Thanks a lot! This looks great. We're currently preparing for a 0.1.0 release, and given the size of the PR we probably don't want to squeeze it in before that release (we're going to be mostly working on stabilizing what we have soon). But this looks like a good candidate to merge early for the release cycle after that one.

JelteF

Some quick cursory review. I'll look at the actual code later.

Tests are failing on PG17. Seemingly because of a changed error message string. If you cannot change the test to create a consistent error message across postgres versions, then it's probably easiest to create a Python test in test/pycheck directory. That way you can change the expected error message based on the postgres version.

test/regression/schedule

src/pgduckdb_types.cpp

JelteF

Overall this seems like a good change. I quite like the refactoring that is done. The only real problem with this implementation is the current usage of SearchSysCache1, which is not thread-safe.

src/pgduckdb_types.cpp

JelteF · 2024-10-30T09:09:49Z

src/pgduckdb_types.cpp

-template <int32_t OID>
-struct PostgresIntegerOIDMapping {};
+static Datum
+ConvertCharDatum(const duckdb::Value &value) {


I very much like these new functions. I think taking that refactor on its own would make the code already a lot better.

JelteF · 2024-10-30T09:11:54Z

src/pgduckdb_types.cpp

+		numeric_var = ConvertNumeric<int64_t>(value.GetValueUnsafe<int64_t>(), scale);
+		break;
+	}
+	case duckdb::PhysicalType::UINT64: {


I know you didn't write this code, just moved it around. But reading this now it makes me wonder. Why is this the only unsigned integer type that we check for the physical type? @mkaruza do you know?

src/pgduckdb_types.cpp

JelteF · 2024-10-30T09:29:15Z

src/pgduckdb_types.cpp

@@ -236,6 +396,16 @@ struct PostgresArrayAppendState {
 		auto &values = duckdb::ListValue::GetChildren(value);
 		idx_t to_append = values.size();

+        if (to_append == 0 && dimension < number_of_dimensions-1) {


This seems to be new code. Could you explain in a comment what edge case this is handling? Because that is not super clear from me from just the code. Also I'm curious why this was not necessary before.

Updated. Use another way to solve this issues.
The origin issue is when we insert empty array into multi-dimension array. The dimension[1] to dimension[number_of_dimensions-1] can not be set because of the logic of AppendValueAtDimension function. So we use the dimension to call the function ConstructArray. It will cause overflow. The reason why it not necessary before is lack of the test cases. When you insert empty array into multi-dimension array. It will happen.

JelteF · 2024-10-30T09:31:24Z

src/pgduckdb_types.cpp

+    if (append_state.expected_values > 0) {
+        pfree(datums);
+        pfree(nulls);
+    }


I don't think this change should be necessary.

I also think the pfree itself can handle this. But when I remove the if condition, and run the tests. I'll get the

CREATE TABLE char_array_2d(a CHAR(1)[][]); INSERT INTO char_array_2d VALUES ('{{"a","b"},{"c","d"}}'), ('{{"e","f","g"},{"h","i","j"}}'), (NULL), ('{{"k","l"},{"m",NULL}}'), ('{}'); SELECT * FROM char_array_2d; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. connection to server was lost

. That's why I add this. It is a little bit strange.

src/pgduckdb_types.cpp

format

…ormance

remove the useless change for clean remove the useless change for clean remove the useless change for clean

Reminiscent · 2024-11-07T01:06:55Z

Some quick cursory review. I'll look at the actual code later.

Tests are failing on PG17. Seemingly because of a changed error message string. If you cannot change the test to create a consistent error message across postgres versions, then it's probably easiest to create a Python test in test/pycheck directory. That way you can change the expected error message based on the postgres version.

Are there any easy way to run the tests in different postgres version in local? I missed the error message in PG17. Can you help me to trigger the tests again? Thanks!

Reminiscent · 2024-11-07T01:47:18Z

@JelteF Thanks for your review! All the comments(except the test failed in PG17) are address. PTAL

JelteF · 2024-11-07T08:14:14Z

Regarding the test failure in PG17, which happens due to a different error message in that PG version. The approach we take so far for cases like this is to move this test to a Python test in the test/pycheck directory.

Reminiscent · 2024-11-07T09:13:13Z

Regarding the test failure in PG17, which happens due to a different error message in that PG version. The approach we take so far for cases like this is to move this test to a Python test in the test/pycheck directory.

I known your meaning. But find the test cases can pass now. Just wonder will you run the regression tests in different postgres version when you develop some new features?

JelteF · 2024-11-07T10:05:57Z

But find the test cases can pass now.

Ah if it passes now you can disregard my comment.

JelteF · 2024-11-12T14:36:35Z

src/pgduckdb_types.cpp

+template <>
+Datum
+PostgresOIDMapping<BOOLOID>::ToDatum(const duckdb::Value &val) {
+	return ConvertBoolDatum(val);
+}


These overrides are unnecessary afaict, the default implementation for PostgresOIDMapping will now do the correct thing.

@JelteF Yes, you are right. Remove the useless part. PTAL.

JelteF · 2024-11-13T09:53:24Z

src/pgduckdb_types.cpp

+	static constexpr bool typbyval = true;
+	static constexpr char typalign = 'c';
+
+	static Datum


Let's mark all these ToDatum functions as static inline. They are so tiny that there's no need for introducing calling overhead.

Let's do the same for some of the smaller Convert functions.

@JelteF Updated. Thanks for your review. PTAL

JelteF

LGTM now. @Y-- do you have some thoughts?

Y--

LGTM, great refactoring + added list support.
I can think of a few more cleanup/refactoring we could do in a separated PR like:

split functions that have big switch/case to return in each case and handle the type mapping/dimensionality separately
use C++ vectors/allocator & raii pattern for allocations rather than palloc

Overall a lot of good stuff, thanks for the PR!

Y-- · 2024-11-13T14:24:16Z

src/pgduckdb_types.cpp

@@ -301,7 +540,6 @@ ConvertDuckToPostgresArray(TupleTableSlot *slot, duckdb::Value &value, idx_t col
 	D_ASSERT(value.type().id() == duckdb::LogicalTypeId::LIST);
 	auto &child_type = GetChildTypeRecursive(value.type());
 	auto child_id = child_type.id();
-	D_ASSERT(child_id == OP::ExpectedType());


Since you've removed the D_ASSERT, you can now remove the unused variable child_id?

Y-- · 2024-11-13T14:28:41Z

src/pgduckdb_types.cpp

@@ -192,6 +169,269 @@ ConvertNumeric(T value, idx_t scale) {
 	return result;
 }

+static Datum
+ConvertNumericDatum(const duckdb::Value &value) {
+	if (value.type().id() == duckdb::LogicalTypeId::DOUBLE) {


nit: we could assign value.type().id() to a const here and re-use it a lot in this function

Reminiscent · 2024-11-14T00:24:37Z

@Y-- Updated. Thanks for your review. PTAL.

Reminiscent · 2024-11-14T00:26:49Z

LGTM, great refactoring + added list support. I can think of a few more cleanup/refactoring we could do in a separated PR like:

split functions that have big switch/case to return in each case and handle the type mapping/dimensionality separately

use C++ vectors/allocator & raii pattern for allocations rather than palloc

Overall a lot of good stuff, thanks for the PR!

I think you are right. After this PR has been merged. I'd like to do this. Thanks!

Y--

LGTM, thanks!

Reminiscent force-pushed the Extended-list-type-support branch 2 times, most recently from 80f9f13 to a3b9268 Compare October 14, 2024 14:22

Reminiscent changed the title ~~[WIP] support extended list types~~ support extended list types Oct 14, 2024

JelteF reviewed Oct 14, 2024

View reviewed changes

test/regression/schedule Outdated Show resolved Hide resolved

src/pgduckdb_types.cpp Show resolved Hide resolved

JelteF added the enhancement New feature or request label Oct 15, 2024

JelteF added this to the 0.2.0 milestone Oct 15, 2024

JelteF requested changes Oct 30, 2024

View reviewed changes

Reminiscent force-pushed the Extended-list-type-support branch 3 times, most recently from 8a99eba to 8445d88 Compare November 7, 2024 00:17

yanchengpeng added 5 commits November 7, 2024 00:41

resolve conflicts

1f18569

resolve conflicts

3148090

format

bfde1c3

format

hardcode the type's information to avoid use SearchSysCache1 for perf…

a28c127

…ormance

remove the useless change for clean

c20ea93

remove the useless change for clean remove the useless change for clean remove the useless change for clean

Reminiscent force-pushed the Extended-list-type-support branch from 8445d88 to c20ea93 Compare November 7, 2024 00:45

set the unset dimensions[i] to 0

33192c9

Reminiscent requested a review from JelteF November 11, 2024 13:06

JelteF added 2 commits November 12, 2024 15:27

Merge branch 'main' into Extended-list-type-support

79002fe

Merge branch 'main' into Extended-list-type-support

5d051ba

JelteF reviewed Nov 12, 2024

View reviewed changes

address comments: remove the useless part

a506391

Reminiscent requested a review from JelteF November 13, 2024 01:26

JelteF reviewed Nov 13, 2024

View reviewed changes

address comments: add the inline keyword to some small functions

1a79391

Reminiscent requested a review from JelteF November 13, 2024 13:43

JelteF approved these changes Nov 13, 2024

View reviewed changes

Y-- approved these changes Nov 13, 2024

View reviewed changes

Reminiscent and others added 2 commits November 14, 2024 08:21

Merge branch 'main' into Extended-list-type-support

4343e07

address comments: remove the unused variable and reuse value_type_id

a775459

Y-- approved these changes Nov 14, 2024

View reviewed changes

Y-- enabled auto-merge (squash) November 14, 2024 07:56

Y-- merged commit d75ed5d into duckdb:main Nov 14, 2024
5 checks passed

saygoodbyye mentioned this pull request Nov 14, 2024

Server crashes if parallel run "type_support" test #425

Open

2 tasks

Reminiscent deleted the Extended-list-type-support branch November 14, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support extended list types #282

support extended list types #282

Reminiscent commented Oct 14, 2024 •

edited

Loading

JelteF commented Oct 14, 2024

JelteF left a comment

JelteF left a comment

JelteF Oct 30, 2024

JelteF Oct 30, 2024

JelteF Oct 30, 2024

Reminiscent Nov 7, 2024

JelteF Oct 30, 2024

Reminiscent Nov 7, 2024 •

edited by JelteF

Loading

Reminiscent commented Nov 7, 2024

Reminiscent commented Nov 7, 2024

JelteF commented Nov 7, 2024 •

edited

Loading

Reminiscent commented Nov 7, 2024

JelteF commented Nov 7, 2024

JelteF Nov 12, 2024 •

edited

Loading

Reminiscent Nov 13, 2024

JelteF Nov 13, 2024 •

edited

Loading

JelteF Nov 13, 2024

Reminiscent Nov 13, 2024

JelteF left a comment

Y-- left a comment

Y-- Nov 13, 2024

Y-- Nov 13, 2024

Reminiscent commented Nov 14, 2024

Reminiscent commented Nov 14, 2024

Y-- left a comment

support extended list types #282

support extended list types #282

Conversation

Reminiscent commented Oct 14, 2024 • edited Loading

JelteF commented Oct 14, 2024

JelteF left a comment

Choose a reason for hiding this comment

JelteF left a 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

Reminiscent Nov 7, 2024 • edited by JelteF Loading

Choose a reason for hiding this comment

Reminiscent commented Nov 7, 2024

Reminiscent commented Nov 7, 2024

JelteF commented Nov 7, 2024 • edited Loading

Reminiscent commented Nov 7, 2024

JelteF commented Nov 7, 2024

JelteF Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF left a comment

Choose a reason for hiding this comment

Y-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reminiscent commented Nov 14, 2024

Reminiscent commented Nov 14, 2024

Y-- left a comment

Choose a reason for hiding this comment

Reminiscent commented Oct 14, 2024 •

edited

Loading

Reminiscent Nov 7, 2024 •

edited by JelteF

Loading

JelteF commented Nov 7, 2024 •

edited

Loading

JelteF Nov 12, 2024 •

edited

Loading

JelteF Nov 13, 2024 •

edited

Loading