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

fix: edge case fix on iceberg target table being dropped instead on being renamed first #667

Merged
merged 12 commits into from
Jun 7, 2024

Conversation

nicor88
Copy link
Member

@nicor88 nicor88 commented Jun 4, 2024

Description

See https://getdbt.slack.com/archives/C013MLFR7BQ/p1717497450266499

Pretty much #625 introduced a regression that was not letting iceberg working in an full high available mode.
This PR cover the issue, and add functional tests to catch these edge case.
Plus it introduce some improvements that allow to work properly in some restrictive LF permission models, where drop can happen only on existing objects.

Models used to test - Optional

See functional tests

Checklist

  • You followed contributing section
  • You kept your Pull Request small and focused on a single feature or bug fix.
  • You added unit testing when necessary
  • You added functional testing when necessary

@nicor88 nicor88 added the enable-functional-tests Label to trigger functional testing label Jun 4, 2024
@nicor88 nicor88 changed the title fix: lint and fix fix: edge case fix on iceberg target table being dropped instead on being renamed first Jun 4, 2024
@nicor88
Copy link
Member Author

nicor88 commented Jun 4, 2024

@octiva might give some input here, as he has a LF env with restricted policies, and we want to be sure that all works fine.

Copy link
Contributor

@octiva octiva left a comment

Choose a reason for hiding this comment

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

@nicor88 I reviewed my previous work, and my changes were in earlier parts of this file: #550.

I think the suggestion ive made about the rename not occurring ever could resolve? Keen to work it out.

@nicor88
Copy link
Member Author

nicor88 commented Jun 5, 2024

@octiva it will be good if you can test this feature again in your environment to see if all works as expected.
Thanks.

@Avinash-1394 I hope that I cover and addressed your question/suggestion. Feel free to have a look.

Copy link
Contributor

@octiva octiva left a comment

Choose a reason for hiding this comment

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

LGTM 🇮🇹

Co-authored-by: Spencer <53303191+octiva@users.noreply.github.com>
@nicor88 nicor88 merged commit aaca397 into main Jun 7, 2024
10 checks passed
@nicor88 nicor88 deleted the fix/iceberg_tables_are_not_ha branch June 7, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enable-functional-tests Label to trigger functional testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants