pg_duckdb icon indicating copy to clipboard operation
pg_duckdb copied to clipboard

support extended list types

Open Reminiscent opened this issue 1 year ago • 1 comments

Related to https://github.com/duckdb/pg_duckdb/issues/101

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

  1. 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.
  1. 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.
  1. Enhanced PostgresArrayAppendState:
  • Modified to handle edge cases, such as empty arrays and NULL values, more robustly.
  1. Updated Test Suite:
  • Expanded the test suite to cover the newly supported array types and dimensions.
  1. 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.

Reminiscent avatar Oct 14 '24 14:10 Reminiscent

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 avatar Oct 14 '24 15:10 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.

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 avatar Nov 07 '24 01:11 Reminiscent

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

Reminiscent avatar Nov 07 '24 01:11 Reminiscent

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.

JelteF avatar Nov 07 '24 08:11 JelteF

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?

Reminiscent avatar Nov 07 '24 09:11 Reminiscent

But find the test cases can pass now.

Ah if it passes now you can disregard my comment.

JelteF avatar Nov 07 '24 10:11 JelteF

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

Reminiscent avatar Nov 14 '24 00:11 Reminiscent

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!

Reminiscent avatar Nov 14 '24 00:11 Reminiscent