Skip to content

Upgrade partition transformer to new pipelines #3064

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 9 commits into from
Apr 13, 2023

Conversation

jachris
Copy link
Contributor

@jachris jachris commented Apr 11, 2023

This PR changes the partition transformer to make use of the new pipelines. Furthermore, there are a few necessary transitive changes, as well as some small additions to the operator API.

@jachris jachris requested a review from a team April 11, 2023 14:08
@jachris jachris marked this pull request as ready for review April 11, 2023 14:08
dominiklohmann
dominiklohmann previously approved these changes Apr 11, 2023
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I think the naive implementation here is fine. I left a few comments, mostly looking for clarification. if the compaction unit tests pass on this one I think it should be good to go.

@dominiklohmann dominiklohmann dismissed their stale review April 12, 2023 10:34

Dismissing as there are further really involved changes that need to be done; discussed earlier in a call.

@jachris jachris force-pushed the topic/upgrade-partition-transformer branch 3 times, most recently from c5fcda6 to bf0de05 Compare April 13, 2023 10:20
@jachris jachris requested a review from dominiklohmann April 13, 2023 10:21
@jachris jachris force-pushed the topic/upgrade-partition-transformer branch from bf0de05 to 32cfb2f Compare April 13, 2023 10:46
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

The tests all pass now, and I tested rebuilding and temporal compaction locally in a minimal setup. Just two places to go now! 🚀

@jachris jachris enabled auto-merge April 13, 2023 14:34
@jachris jachris merged commit b799871 into main Apr 13, 2023
@jachris jachris deleted the topic/upgrade-partition-transformer branch April 13, 2023 16:32
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