Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fails value insertion if type unsupported by duckdb #471

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions src/pgduckdb_ddl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,45 @@ static bool ctas_skip_data = false;
static bool top_level_ddl = true;
static ProcessUtility_hook_type prev_process_utility_hook = NULL;

// Get precision and scale from postgres type modifier.
static int
GetPgNumericPrecision(int32 typmod) {
return ((typmod - VARHDRSZ) >> 16) & 0xffff;
}
static int
GetPgNumericScale(int32 typmod) {
return (((typmod - VARHDRSZ) & 0x7ff) ^ 1024) - 1024;
}

// If column is of type "numeric", check whether it's acceptable for duckdb;
// If not, exception is thrown via `elog`.
static void
ValidateColumnNumericType(Form_pg_attribute &attribute) {
auto &type = attribute->atttypid;
if (type != NUMERICOID) {
return;
}
auto &typmod = attribute->atttypmod;
auto precision = GetPgNumericPrecision(typmod);
auto scale = GetPgNumericScale(typmod);
// duckdb's "numeric" type's max supported precision is 38.
if (typmod == -1 || precision < 0 || scale < 0 || precision > 38) {
elog(ERROR,
"Unsupported type when creating column: type precision %d with scale %d is not supported by duckdb, which "
dentiny marked this conversation as resolved.
Show resolved Hide resolved
"only allows maximum precision 38",
precision, scale);
}
}

// Validate new relation creation, if invalid throw exception via `elog`.
static void
ValidateRelationCreation(TupleDesc desc) {
for (int i = 0; i < desc->natts; i++) {
Form_pg_attribute attr = &desc->attrs[i];
ValidateColumnNumericType(attr);
}
}

static void
DuckdbHandleDDL(Node *parsetree) {
if (!pgduckdb::IsExtensionRegistered()) {
Expand Down Expand Up @@ -253,6 +292,10 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) {
elog(ERROR, "Expected single table to be created, but found %" PRIu64, static_cast<uint64_t>(SPI_processed));
}

// Check whether new table creation is supported in duckdb.
// TODO(hjiang): Same type validation should be applied to other DDL as well.
ValidateRelationCreation(SPI_tuptable->tupdesc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prob move this check to pgduckdb_get_tabledef(), where other lockdowns (e.g., generated columns, identity columns) are handled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense, my concern is, other DDL statements (i.e. ALTER TABLE) require the same validation logic.
I would prefer to put the validation logic in the same file.

Copy link
Contributor Author

@dentiny dentiny Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where other lockdowns (e.g., generated columns, identity columns) are handled

I fully agree to place all validation logic into one function / one place.
My personal preference is to move all logic into DDL file, since other validations (i.e. generated columns, as you mentioned) should also apply to other DDL statements.

Would like to know how pg_duckdb's owner think about it. :)


HeapTuple tuple = SPI_tuptable->vals[0];
bool isnull;
Datum relid_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 1, &isnull);
Expand Down
4 changes: 2 additions & 2 deletions test/regression/expected/array_type_support.out
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ SELECT * FROM float8_array_1d;
(4 rows)

-- NUMERIC (single dimension)
CREATE TABLE numeric_array_1d(a NUMERIC[]);
CREATE TABLE numeric_array_1d(a NUMERIC(2, 1)[]);
INSERT INTO numeric_array_1d SELECT CAST(a as NUMERIC[]) FROM (VALUES
('{1.1, 2.2, 3.3}'),
(NULL),
Expand Down Expand Up @@ -373,7 +373,7 @@ SELECT * FROM float8_array_2d;
(5 rows)

-- NUMERIC (two dimensions)
CREATE TABLE numeric_array_2d(a NUMERIC[][]);
CREATE TABLE numeric_array_2d(a NUMERIC(3, 1)[][]);
INSERT INTO numeric_array_2d VALUES
('{{1.1,2.2},{3.3,4.4}}'),
('{{5.5,6.6,7.7},{8.8,9.9,10.1}}'),
Expand Down
16 changes: 0 additions & 16 deletions test/regression/expected/type_support.out
Original file line number Diff line number Diff line change
Expand Up @@ -201,21 +201,6 @@ SELECT * FROM float8_tbl;
4.582345020342342e+20
(3 rows)

-- NUMERIC as DOUBLE
CREATE TABLE numeric_as_double(a NUMERIC);
INSERT INTO numeric_as_double SELECT a FROM (VALUES
(0.234234234),
(NULL),
(458234502034234234234.000012)
) t(a);
SELECT * FROM numeric_as_double;
a
-----------------------
0.234234234

4.582345020342342e+20
(3 rows)

-- NUMERIC with a physical type of SMALLINT
CREATE TABLE smallint_numeric(a NUMERIC(4, 2));
INSERT INTO smallint_numeric SELECT a FROM (VALUES
Expand Down Expand Up @@ -330,7 +315,6 @@ DROP TABLE timestamp_tbl;
DROP TABLE timestamptz_tbl;
DROP TABLE float4_tbl;
DROP TABLE float8_tbl;
DROP TABLE numeric_as_double;
DROP TABLE smallint_numeric;
DROP TABLE integer_numeric;
DROP TABLE bigint_numeric;
Expand Down
3 changes: 3 additions & 0 deletions test/regression/expected/unsupported_types.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
CREATE TEMP TABLE large_numeric_tbl (a NUMERIC) USING duckdb;
INSERT INTO large_numeric_tbl VALUES(856324.111122223333::NUMERIC(40,12));
ERROR: (PGDuckDB/CreatePlan) Prepared query returned an error: 'Parser Error: Width must be between 1 and 38!
1 change: 1 addition & 0 deletions test/regression/schedule
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ test: transaction_errors
test: secrets
test: prepare
test: function
test: unsupported_types
4 changes: 2 additions & 2 deletions test/regression/sql/array_type_support.sql
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ INSERT INTO float8_array_1d SELECT CAST(a as FLOAT8[]) FROM (VALUES
SELECT * FROM float8_array_1d;

-- NUMERIC (single dimension)
CREATE TABLE numeric_array_1d(a NUMERIC[]);
CREATE TABLE numeric_array_1d(a NUMERIC(2, 1)[]);
INSERT INTO numeric_array_1d SELECT CAST(a as NUMERIC[]) FROM (VALUES
('{1.1, 2.2, 3.3}'),
(NULL),
Expand Down Expand Up @@ -226,7 +226,7 @@ INSERT INTO float8_array_2d VALUES
SELECT * FROM float8_array_2d;

-- NUMERIC (two dimensions)
CREATE TABLE numeric_array_2d(a NUMERIC[][]);
CREATE TABLE numeric_array_2d(a NUMERIC(3, 1)[][]);
INSERT INTO numeric_array_2d VALUES
('{{1.1,2.2},{3.3,4.4}}'),
('{{5.5,6.6,7.7},{8.8,9.9,10.1}}'),
Expand Down
10 changes: 0 additions & 10 deletions test/regression/sql/type_support.sql
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,6 @@ INSERT INTO float8_tbl SELECT CAST(a AS FLOAT8) FROM (VALUES
) t(a);
SELECT * FROM float8_tbl;

-- NUMERIC as DOUBLE
CREATE TABLE numeric_as_double(a NUMERIC);
INSERT INTO numeric_as_double SELECT a FROM (VALUES
(0.234234234),
(NULL),
(458234502034234234234.000012)
) t(a);
SELECT * FROM numeric_as_double;

-- NUMERIC with a physical type of SMALLINT
CREATE TABLE smallint_numeric(a NUMERIC(4, 2));
INSERT INTO smallint_numeric SELECT a FROM (VALUES
Expand Down Expand Up @@ -171,7 +162,6 @@ DROP TABLE timestamp_tbl;
DROP TABLE timestamptz_tbl;
DROP TABLE float4_tbl;
DROP TABLE float8_tbl;
DROP TABLE numeric_as_double;
DROP TABLE smallint_numeric;
DROP TABLE integer_numeric;
DROP TABLE bigint_numeric;
Expand Down
2 changes: 2 additions & 0 deletions test/regression/sql/unsupported_types.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE TEMP TABLE large_numeric_tbl (a NUMERIC) USING duckdb;
INSERT INTO large_numeric_tbl VALUES(856324.111122223333::NUMERIC(40,12));