Skip to content

Make time partition suffix customizable #200

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

Merged
merged 1 commit into from
Mar 25, 2023

Conversation

ckrybus
Copy link
Contributor

@ckrybus ckrybus commented Feb 11, 2023

An implementation attempt for #197

Usage example:

manager = PostgresPartitioningManager(
    [
        PostgresPartitioningConfig(
            model=PartitionedMonthlyModel,
            strategy=CustomPostgresCurrentTimePartitioningStrategy(
                size=PostgresTimePartitionSize(months=1),
                count=3,
                name_format="%Y_%m",     # <--- new functionality
            ),
        ),
    ]
)

The change is fairly clean and it works, but design-wise I'm not sure if it is the right approach. Another possibility would be to add is as PostgresTimePartitionSize parameter, because the new parameter is "coupled to a specific size" (e.g. using %Y_%m_%d together with PostgresTimePartitionSize(months=1) does not really make sense). That said, PostgresTimePartitionSize is not responsible for naming, so 🤷‍♂️

@ckrybus ckrybus force-pushed the customizable-partition-naming branch from 212b1f9 to d37bdb4 Compare February 11, 2023 12:20
@ckrybus ckrybus marked this pull request as ready for review March 15, 2023 17:07
@Photonios
Copy link
Member

Looks acceptable to me! The fact that you might shoot yourself in the foot by doing %Y_%m_%d with PostgresTimePartitionSize(months=1) is acceptable to me. I suppose you know what you're doing if you use this parameter.

@Photonios Photonios merged commit 2adc13f into SectorLabs:master Mar 25, 2023
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