Move table splitting logic to SQL #3119

mathemancer · 2023-08-01T14:33:48Z

Related to #2737

This PR moves the logic for splitting tables by extracting chosen columns to the database. It also moves the tests to SQL tests.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the develop branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

This less-strict stance will make it easier to get things working in the python layer for now. We should reconsider later.

silentninja

Looks good to me! I have made some requests to improve readability, please add it to the PR and merge it.

silentninja · 2023-08-03T17:50:41Z

db/sql/0_msar.sql

+    'not_null', attnotnull,
+    'default',
+    -- We only copy non-dynamic default expressions to new table to avoid double-use of sequences.
+    CASE WHEN NOT msar.is_default_possibly_dynamic(tab_id, col_id) THEN


Does this mean we lose the dynamic default information in case a column has one?

Yes, for now. We should probably add that as an issue. I believe we were already dropping those.

I'll clarify the comment. The issue is that in the case of sequence dynamic defaults, the sequence is owned by the column that uses it, and can't be reused by any other column. It'll throw an error if you try. but, our dynamic default detection isn't (yet) sophisticated enough to detect when this is the case, and we shouldn't copy it, or when it's something safe like NOW().

silentninja · 2023-08-03T18:19:41Z

db/sql/0_msar.sql

@@ -2087,18 +2195,18 @@ $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;

 CREATE OR REPLACE FUNCTION
 msar.create_many_to_one_link(
-  from_rel_id oid,
-  to_rel_id oid,
+  frel_id oid,


I prefer the from_rel_id better as it informs the direction of the link. Any reason for renaming it?

Well, it was backwards, so it must not have helped that much 😄. I.e., the foreign key was from to_rel_id to from_rel_id. The reason I changed it was to match up with what's in the pg_attribute table, and related terminology. I haven't actually found a good name, so I figure it makes most sense to match up with what is in the PG catalog.

I added more in the docstring to that effect.

silentninja · 2023-08-03T18:49:07Z

db/sql/0_msar.sql

+  fkey_attnum := msar.create_many_to_one_link(extracted_table_id, tab_id, fkey_name);
+  PERFORM __msar.exec_ddl($t$
+    WITH fkey_cte AS (
+      SELECT id, %1$s, dense_rank() OVER (ORDER BY %1$s) AS __msar_tmp_id


It will be helpful to add comments on what the placeholders refer to. Something like

-- %1$s refers to the columns to be moved

Done; I also added a number of other comments describing the process.

mathemancer added 14 commits July 27, 2023 12:18

allow choosing whether to set the NOTNULL value

35a8019

rename variables for clarity and consistency with PG

c94b71c

first pass table splitting in SQL

709d7fc

add more info to return value

6f4690a

back down column generation to BY DEFAULT

3214866

This less-strict stance will make it easier to get things working in the python layer for now. We should reconsider later.

remove useless cruft

acd92db

update tests for new function signatures

5707ebb

move function locations for unique colname builders

64fce41

Merge branch 'develop' into split_tables_sql

79d4b64

Merge branch 'develop' into split_tables_sql

6a31cb1

move table splitting tests to SQL

9de3df0

add docstrings for column name builders

6e54a15

revert some changes which are no longer needed

df4f66b

tidy and complete final documentation

22bd783

mathemancer marked this pull request as ready for review August 1, 2023 14:57

mathemancer requested a review from silentninja August 1, 2023 14:57

mathemancer assigned silentninja Aug 1, 2023

mathemancer added the pr-status: review A PR awaiting review label Aug 1, 2023

kgodey added this to the v0.1.3 milestone Aug 1, 2023

Merge branch 'develop' into split_tables_sql

2121140

silentninja approved these changes Aug 3, 2023

View reviewed changes

silentninja added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Aug 3, 2023

silentninja assigned mathemancer and unassigned silentninja Aug 3, 2023

mathemancer added 2 commits August 4, 2023 12:39

add numerous comments documenting confusing bits of code

8816a4d

Merge branch 'develop' into split_tables_sql

cf9bfaa

mathemancer enabled auto-merge August 4, 2023 04:41

mathemancer added this pull request to the merge queue Aug 4, 2023

Merged via the queue into develop with commit ab68686 Aug 4, 2023
16 checks passed

mathemancer deleted the split_tables_sql branch August 4, 2023 05:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move table splitting logic to SQL #3119

Move table splitting logic to SQL #3119

mathemancer commented Aug 1, 2023

silentninja left a comment

silentninja Aug 3, 2023

mathemancer Aug 4, 2023

mathemancer Aug 4, 2023

silentninja Aug 3, 2023

mathemancer Aug 4, 2023 •

edited

mathemancer Aug 4, 2023

silentninja Aug 3, 2023

mathemancer Aug 4, 2023

Move table splitting logic to SQL #3119

Move table splitting logic to SQL #3119

Conversation

mathemancer commented Aug 1, 2023

Checklist

Developer Certificate of Origin

silentninja left a comment

Choose a reason for hiding this comment

silentninja Aug 3, 2023

Choose a reason for hiding this comment

mathemancer Aug 4, 2023

Choose a reason for hiding this comment

mathemancer Aug 4, 2023

Choose a reason for hiding this comment

silentninja Aug 3, 2023

Choose a reason for hiding this comment

mathemancer Aug 4, 2023 • edited

Choose a reason for hiding this comment

mathemancer Aug 4, 2023

Choose a reason for hiding this comment

silentninja Aug 3, 2023

Choose a reason for hiding this comment

mathemancer Aug 4, 2023

Choose a reason for hiding this comment

mathemancer Aug 4, 2023 •

edited