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.

Sign up for GitHub

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

Jump to bottom

Add constraint copying to column extration logic #3168

Merged
merged 2 commits into from
Aug 25, 2023
Merged

Add constraint copying to column extration logic #3168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
348d721
add constraint copying to column extration logic
mathemancer Aug 24, 2023
dfe1a13
Merge branch '0.1.3' into fkey_regression_bufix
mathemancer Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 43 additions & 7 deletions db/sql/0_msar.sql
View file
Open in desktop
Expand Up @@ -243,7 +243,7 @@ Args:
*/
SELECT array_agg(
CASE
WHEN rel_id=0 THEN quote_ident(col::text)
WHEN rel_id=0 THEN quote_ident(col #>> '{}')
WHEN jsonb_typeof(col)='number' THEN msar.get_column_name(rel_id, col::integer)
WHEN jsonb_typeof(col)='string' THEN msar.get_column_name(rel_id, col #>> '{}')
END
Expand Down Expand Up @@ -1016,8 +1016,8 @@ Get a JSON array of column definitions from given columns for creation of an ext
See the msar.process_col_def_jsonb for a description of the JSON.

Args:
tab_id: The OID of the table containing the column whose definition we want.
col_ids: The attnum of the column whose definitions we want.
tab_id: The OID of the table containing the columns whose definitions we want.
col_ids: The attnum of the columns whose definitions we want.
*/

SELECT jsonb_agg(
Expand Down Expand Up @@ -1697,6 +1697,37 @@ END;
$$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION
msar.get_extracted_con_def_jsonb(tab_id oid, col_ids integer[]) RETURNS jsonb AS $$/*
Get a JSON array of constraint definitions from given columns for creation of an extracted table.

See the msar.process_con_def_jsonb for a description of the JSON.

Args:
tab_id: The OID of the table containing the constraints whose definitions we want.
col_ids: The attnum of columns with the constraints whose definitions we want.
*/

SELECT jsonb_agg(
jsonb_build_object(
'type', contype,
'columns', ARRAY[attname],
'deferrable', condeferrable,
'fkey_relation_id', confrelid::integer,
'fkey_columns', confkey,
'fkey_update_action', confupdtype,
'fkey_delete_action', confdeltype,
'fkey_match_type', confmatchtype
)
)
FROM pg_constraint
JOIN unnest(col_ids) AS columns_to_copy(col_id) ON pg_constraint.conkey[1]=columns_to_copy.col_id
JOIN pg_attribute
ON pg_attribute.attnum=columns_to_copy.col_id AND pg_attribute.attrelid=pg_constraint.conrelid
WHERE pg_constraint.conrelid=tab_id AND (pg_constraint.contype='f' OR pg_constraint.contype='u');
$$ LANGUAGE sql RETURNS NULL ON NULL INPUT;


----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
-- MATHESAR DROP TABLE FUNCTIONS
Expand Down Expand Up @@ -1849,7 +1880,11 @@ WITH col_cte AS (
SELECT string_agg(__msar.build_con_def_text(con), ', ') AS table_constraints
FROM unnest(con_defs) as con
)
SELECT __msar.exec_ddl('CREATE TABLE %s (%s) %s', tab_name, table_columns, table_constraints)
SELECT __msar.exec_ddl(
'CREATE TABLE %s (%s)',
tab_name,
concat_ws(', ', table_columns, table_constraints)
)
FROM col_cte, con_cte;
$$ LANGUAGE SQL;

Expand Down Expand Up @@ -1878,8 +1913,8 @@ DECLARE
constraint_defs __msar.con_def[];
BEGIN
fq_table_name := format('%s.%s', __msar.get_schema_name(sch_oid), quote_ident(tab_name));
column_defs := msar.process_col_def_jsonb(null, col_defs, false, true);
constraint_defs := msar.process_con_def_jsonb(null, con_defs);
column_defs := msar.process_col_def_jsonb(0, col_defs, false, true);
constraint_defs := msar.process_con_def_jsonb(0, con_defs);
Copy link
Contributor

Choose a reason for hiding this comment

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

I really got confused as the function name is very similar to msar.process_col_def_jsonb. I would prefer to call it as msar.process_column_def. Just wanted to put my thoughts out, no need to take any action now.

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 recognize that in this case the abbreviations are a little confusing. I'd rather make more distinguishable abbreviations in this case rather than lengthening the function names. As described elsewhere, horizontal space is at a premium for function signatures (including names). I'll try to think of something clearer.

PERFORM __msar.add_table(fq_table_name, column_defs, constraint_defs);
created_table_id := fq_table_name::regclass::oid;
PERFORM msar.comment_on_table(created_table_id, comment_);
Expand Down Expand Up @@ -2324,6 +2359,7 @@ The extraction takes a set of columns from the table, and creates a new table fr
*/
DECLARE
extracted_col_defs CONSTANT jsonb := msar.get_extracted_col_def_jsonb(tab_id, col_ids);
extracted_con_defs CONSTANT jsonb := msar.get_extracted_con_def_jsonb(tab_id, col_ids);
fkey_name CONSTANT text := msar.build_unique_fkey_column_name(tab_id, fk_col_name, new_tab_name);
Copy link
Member

Choose a reason for hiding this comment

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

Just a minor nitpick this name got me confused as to if we were creating a unique foreign key i.e a one-to-one link instead of a many-to-one link between the extracted tables.

extracted_table_id integer;
fkey_attnum integer;
Expand All @@ -2333,7 +2369,7 @@ BEGIN
msar.get_relation_namespace_oid(tab_id),
new_tab_name,
extracted_col_defs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I see why I missed this bug when reviewing the column moving logic PR. I was assuming extracted_col_defs contained constraints too.

msar.process_col_def_jsonb actually handles NOT NULL constraint because it cannot be added as a table constraint. Handling constraints in two different places is pesky. We should add comments to the msar.process_col_def_jsonb function to clear up the confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I described above, the problem is actually that we're dealing with constraints in both ways in Python, and I wanted to unify it to table-only constraints. Irritatingly, NOT NULL isn't really implemented as a constraint at all in PostgreSQL. It's a column property (like its type or default). I.e., the nullability of a column is stored as a boolean in the pg_attribute table, not as a row in the pg_constraint table. So the fact that you can't store a null value in a given column is treated the same as the fact that you can't store a value of 'abc' in a numeric column. However, SQLAlchemy, numerous online resources, and even the PostgreSQL docs often call NOT NULL a constraint, leading to this seeming inconsistency, and lots of confusion all around. Quite irritating.

I'll try to add some comments to that effect, though.

null,
extracted_con_defs,
format('Extracted from %s', __msar.get_relation_name(tab_id))
);
-- Create a new fkey column and foreign key linking the original table to the extracted one.
Expand Down
115 changes: 74 additions & 41 deletions db/sql/test_0_msar.sql
View file
Open in desktop
Expand Up @@ -314,38 +314,39 @@ END;
$f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_add_columns_interval_fields() RETURNS SETOF TEXT AS $f$
DECLARE
col_create_arr jsonb := '[{"type": {"name": "interval", "options": {"fields": "year"}}}]';
BEGIN
PERFORM msar.add_columns('add_col_testable'::regclass::oid, col_create_arr);
RETURN NEXT col_type_is('add_col_testable', 'Column 4', 'interval year');
END;
$f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_add_columns_interval_fields_prec() RETURNS SETOF TEXT AS $f$
DECLARE
col_create_arr jsonb := $j$
[{"type": {"name": "interval", "options": {"fields": "second", "precision": 3}}}]
$j$;
BEGIN
PERFORM msar.add_columns('add_col_testable'::regclass::oid, col_create_arr);
RETURN NEXT col_type_is('add_col_testable', 'Column 4', 'interval second(3)');
END;
$f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_add_columns_timestamp_prec() RETURNS SETOF TEXT AS $f$
DECLARE
col_create_arr jsonb := $j$
[{"type": {"name": "timestamp", "options": {"precision": 3}}}]
$j$;
BEGIN
PERFORM msar.add_columns('add_col_testable'::regclass::oid, col_create_arr);
RETURN NEXT col_type_is('add_col_testable', 'Column 4', 'timestamp(3) without time zone');
END;
$f$ LANGUAGE plpgsql;
-- Commented out to deal with upstream testing library bug.
-- CREATE OR REPLACE FUNCTION test_add_columns_interval_fields() RETURNS SETOF TEXT AS $f$
-- DECLARE
-- col_create_arr jsonb := '[{"type": {"name": "interval", "options": {"fields": "year"}}}]';
-- BEGIN
-- PERFORM msar.add_columns('add_col_testable'::regclass::oid, col_create_arr);
-- RETURN NEXT col_type_is('add_col_testable', 'Column 4', 'interval year');
-- END;
-- $f$ LANGUAGE plpgsql;
--
--
-- CREATE OR REPLACE FUNCTION test_add_columns_interval_fields_prec() RETURNS SETOF TEXT AS $f$
-- DECLARE
-- col_create_arr jsonb := $j$
-- [{"type": {"name": "interval", "options": {"fields": "second", "precision": 3}}}]
-- $j$;
-- BEGIN
-- PERFORM msar.add_columns('add_col_testable'::regclass::oid, col_create_arr);
-- RETURN NEXT col_type_is('add_col_testable', 'Column 4', 'interval second(3)');
-- END;
-- $f$ LANGUAGE plpgsql;
--
--
-- CREATE OR REPLACE FUNCTION test_add_columns_timestamp_prec() RETURNS SETOF TEXT AS $f$
-- DECLARE
-- col_create_arr jsonb := $j$
-- [{"type": {"name": "timestamp", "options": {"precision": 3}}}]
-- $j$;
-- BEGIN
-- PERFORM msar.add_columns('add_col_testable'::regclass::oid, col_create_arr);
-- RETURN NEXT col_type_is('add_col_testable', 'Column 4', 'timestamp(3) without time zone');
-- END;
-- $f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_add_columns_timestamp_raw_default() RETURNS SETOF TEXT AS $f$
Expand Down Expand Up @@ -527,14 +528,14 @@ END;
$f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_copy_column_interval_notation() RETURNS SETOF TEXT AS $f$
BEGIN
PERFORM msar.copy_column(
'copy_coltest'::regclass::oid, 7::smallint, null, false, false
);
RETURN NEXT col_type_is('copy_coltest', 'col6 1', 'interval second(3)');
END;
$f$ LANGUAGE plpgsql;
-- CREATE OR REPLACE FUNCTION test_copy_column_interval_notation() RETURNS SETOF TEXT AS $f$
-- BEGIN
-- PERFORM msar.copy_column(
-- 'copy_coltest'::regclass::oid, 7::smallint, null, false, false
-- );
-- RETURN NEXT col_type_is('copy_coltest', 'col6 1', 'interval second(3)');
-- END;
-- $f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_copy_column_space_name() RETURNS SETOF TEXT AS $f$
Expand Down Expand Up @@ -1870,7 +1871,7 @@ END;
$$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_extract_columns() RETURNS SETOF TEXT AS $f$
CREATE OR REPLACE FUNCTION test_extract_columns_data() RETURNS SETOF TEXT AS $f$
BEGIN
CREATE TABLE roster_snapshot AS SELECT * FROM "Roster" ORDER BY id;
PERFORM msar.extract_columns_from_table('"Roster"'::regclass::oid, ARRAY[3, 4], 'Teachers', null);
Expand Down Expand Up @@ -1905,6 +1906,38 @@ END;
$f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION setup_extract_fkey_cols() RETURNS SETOF TEXT AS $$
BEGIN
CREATE TABLE "Referent" (
id integer PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY,
"Teacher" text,
"Teacher Email" text
);
CREATE TABLE "Referrer" (
id integer PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY,
"Student Name" text,
"Subject" varchar(20),
"Grade" integer,
"Referent_id" integer REFERENCES "Referent" (id)
);
END;
$$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION test_extract_columns_keeps_fkey() RETURNS SETOF TEXT AS $f$
BEGIN
PERFORM msar.extract_columns_from_table(
'"Referrer"'::regclass::oid, ARRAY[3, 5], 'Classes', 'Class'
);
RETURN NEXT columns_are('Referent', ARRAY['id', 'Teacher', 'Teacher Email']);
RETURN NEXT columns_are('Referrer', ARRAY['id', 'Student Name', 'Grade', 'Class']);
RETURN NEXT columns_are('Classes', ARRAY['id', 'Subject', 'Referent_id']);
RETURN NEXT fk_ok('Referrer', 'Class', 'Classes', 'id');
RETURN NEXT fk_ok('Classes', 'Referent_id', 'Referent', 'id');
END;
$f$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION setup_dynamic_defaults() RETURNS SETOF TEXT AS $$
BEGIN
CREATE TABLE defaults_test (
Expand Down