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

Disallow out of range numerics type on table creation #49

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 4, 2024

Followup PR for issue #47
This PR disallows table creation when out-of-range (for duckdb) numerics types created.

The implementation here copies the type checking logic into columnstore_handle.cpp.
Pro: Having the check logic outside of pg_duckdb reduces the overhead for upgrade
Con: Need to maintain two util functions to get scale and width, every time we upgrade pg_duckdb.

Alternative considered:
Method-1: add extra type conversion code in pg_duckdb.
Example code

if (scale > 38) return duckdb::USER(...)

Pro: least possible code change
Con: every time we upgrade pg_duckdb, the extra checking logic has to be cherry-picked also

Method-2: propagate the logic to table creation logic into pg_duckdb
It doesn't work because pg_duckdb doesn't support general-purpose table creation as of now;
Moving the logic into pg type -> duck type conversion is also not proper.

How I tested:

postgres (pid: 32200) =# CREATE TABLE mooncake_numeric_tbl_2 (val numeric(40, 5)) USING columnst
ore;
ERROR:  Unsupported type when creating column: type precision 40 with scale 5 is not supported by duckdb
postgres (pid: 32200) =# CREATE TABLE mooncake_numeric_tbl_2 (val numeric) USING columnstore;
ERROR:  Unsupported type when creating column: type precision 65535 with scale -5 is not supported by duckdb
postgres (pid: 32200) =# CREATE TABLE mooncake_numeric_tbl_2 (val numeric(38, 3)) USING columnst
ore;
CREATE TABLE

@dentiny dentiny force-pushed the hjiang/disallow-out-of-range-numerics branch 7 times, most recently from d49ad88 to 003b9f6 Compare December 4, 2024 09:01
@dentiny dentiny changed the title Disallow out of range numerics type Disallow out of range numerics type on table creation Dec 4, 2024
@dentiny dentiny force-pushed the hjiang/disallow-out-of-range-numerics branch from 003b9f6 to ae72d4f Compare December 4, 2024 10:23
src/columnstore_handler.cpp Outdated Show resolved Hide resolved
src/columnstore_handler.cpp Outdated Show resolved Hide resolved

// If column is of type "numeric", check whether it's acceptable for mooncake;
// If not, exception is thrown via `elog`.
void ValidateColumnNumericType(Form_pg_attribute &attribute) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems good to also have the same lockdown for pg_duckdb's temp DuckDB table?
So it might be better to make this change to pg_duckdb instead?

Copy link
Contributor Author

@dentiny dentiny Dec 5, 2024

Choose a reason for hiding this comment

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

If I don't read it wrong, this is how pg_duckdb handles temp table creation:
https://github.com/duckdb/pg_duckdb/blob/bb82c937936b97824bb766893745e1456eb1aa08/src/pgduckdb_ddl.cpp#L199-L364

Considering the fact that pg_duckdb doesn't support complete table creation functionality, I think it would be nice if:

  • pg_duckdb locks down temp table creation on duckdb-incompatible types
  • pg_mooncake locks down table creation on duckdb-incompatible types

It seems good to also have the same lockdown for pg_duckdb's temp DuckDB table?

To your question, yes.

So it might be better to make this change to pg_duckdb instead?

But I'm not sure how we could place the whole lock down logic into pg_duckdb?
Since it only handles part of the table creation logic as of now.

I mentioned it briefly in the PR description:

Method-2: propagate the logic to table creation logic into pg_duckdb
It doesn't work because pg_duckdb doesn't support general-purpose table creation as of now;
Moving the logic into pg type -> duck type conversion is also not proper.

Let me know if I mis-understand pg_duckdb's capability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the creation of columnstore tables and DuckDB temporary tables follow different code paths
My suggestion was only to move the helper functions numeric_typmod_precision, numeric_typmod_scale, and ValidateColumnNumericType in this file to pg_duckdb
I noticed you just opened duckdb/pg_duckdb#471, which already addresses this
We just need to wait for that diff to land and re-pull from pg_duckdb

@dentiny dentiny force-pushed the hjiang/disallow-out-of-range-numerics branch 2 times, most recently from 324f0e1 to a3110b8 Compare December 5, 2024 12:27
@dentiny dentiny force-pushed the hjiang/disallow-out-of-range-numerics branch 2 times, most recently from 9ff12f0 to 677e521 Compare December 5, 2024 12:46
@dentiny dentiny requested a review from dpxcc December 5, 2024 12:55
Copy link
Contributor

@dpxcc dpxcc left a comment

Choose a reason for hiding this comment

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

We just need to wait for duckdb/pg_duckdb#471 to land and re-pull from pg_duckdb


// If column is of type "numeric", check whether it's acceptable for mooncake;
// If not, exception is thrown via `elog`.
void ValidateColumnNumericType(Form_pg_attribute &attribute) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the creation of columnstore tables and DuckDB temporary tables follow different code paths
My suggestion was only to move the helper functions numeric_typmod_precision, numeric_typmod_scale, and ValidateColumnNumericType in this file to pg_duckdb
I noticed you just opened duckdb/pg_duckdb#471, which already addresses this
We just need to wait for that diff to land and re-pull from pg_duckdb

test/expected/unsupported/types.out Show resolved Hide resolved
@dentiny dentiny force-pushed the hjiang/disallow-out-of-range-numerics branch from 64edfb9 to 3dc77c4 Compare December 8, 2024 10:49
@dpxcc dpxcc force-pushed the main branch 3 times, most recently from 64a3e22 to 6a655ea Compare January 15, 2025 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants