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

feat: support for composite/sub partitioning #202

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

quertenmont
Copy link
Contributor

Hello,

I needed to have composite partitioning (aka nested or subpartitioning),
I made the changes to make it work for one level subpartitioning.

The principal changes are:

  1. Handling "submethod" and "subkey" variable in the partitioning meta object
  2. Adding partition_by and parent_partition_name in the PostgresPartition class to make it aware that it could have either parent or child partition (and add the PARTITION OF and PARTITION BY sql statement where needed)
  3. Adding a submethod in the Config to handle how the second level should be created
  4. Updating the manager to create a second level of partition if needed

I tested the code with TimePartitioning at first level and a List partitioning at second level, it's working nicely.

The code could certainly be improved, but the functionality is there and it would certainly be useful to others to have it in the main package. Thanks for considering its inclusion.

Copy link
Member

@Photonios Photonios left a comment

Choose a reason for hiding this comment

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

Thanks for giving it a shot!

There are some items that prevent me from considering inclusions at this time:

  1. PR re-formats unrelated code.
  2. Lack of unit tests.
  3. Left-over prints and debugging code.

I am happy to leave the PR open for the time being so it can be improved or to serve as inspiration for others wishing to do the same.

Comment on lines +50 to +52
def execute(self, sql, params=()):
"""execute query"""
return super().execute(sql, params)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that could actually be removed.
I added that to print sql statement and debug my code implementation.

@quertenmont
Copy link
Contributor Author

Yes, the main goal of the PR was to give it over to someone else to polish and commission the code for inclusion :-)

I don't have much time nor patience to do it myself :-)

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