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

ADAP-1017: Fix configuration change monitoring for scenarios with no changes #1009

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

mikealfare
Copy link
Contributor

resolves #1007

Problem

Users cannot recreate a materialized view if it does not have any changes. The configuration change collection is not being recognized as an empty set of changes.

Solution

The check currently looks for the existence of the config change collection instead of whether it has any changes. Update the check to look for the existence of changes instead of the object itself.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@mikealfare mikealfare self-assigned this Nov 7, 2023
@mikealfare mikealfare requested a review from a team as a code owner November 7, 2023 23:03
@cla-bot cla-bot bot added the cla:yes label Nov 7, 2023
@mikealfare mikealfare removed the request for review from nathaniel-may November 7, 2023 23:04
VersusFacit
VersusFacit previously approved these changes Nov 7, 2023
Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

oh my.

Feel free to merge assuming we don't need a if config_change_collection and config_change_collection.has_changes style guard.

You know the method better than me, so act with that discretion.

@VersusFacit VersusFacit dismissed their stale review November 7, 2023 23:48

test failures cropped up

@mikealfare
Copy link
Contributor Author

assuming we don't need a if config_change_collection and config_change_collection.has_changes style guard.

Just to respond to this, config_change_collection is instantiated as an empty BigQueryMaterializedViewConfigChangeset, which has a has_changes calculated property.

@mikealfare mikealfare merged commit 64e042a into main Nov 8, 2023
14 checks passed
@mikealfare mikealfare deleted the materialized-views/bug/adap-1017 branch November 8, 2023 01:47
@idoshamun
Copy link

@mikealfare @VersusFacit when do you expect to release it?
I'm eagerly waiting for this one 🙏

@yinkaihsu
Copy link

Also waiting for this release 🙏

mikealfare added a commit that referenced this pull request Jan 29, 2024
…changes (#1009)

* changie
* add a test demonstrating the issue
* check that the change collection has changes instead of checking that it exists
* remove the partition config value from the config change since it is not hashable

(cherry picked from commit 64e042a)
mikealfare added a commit that referenced this pull request Jan 29, 2024
…changes (#1009) (#1078)

* changie
* add a test demonstrating the issue
* check that the change collection has changes instead of checking that it exists
* remove the partition config value from the config change since it is not hashable

(cherry picked from commit 64e042a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ADAP-1017] [Bug] [1.7] materialized views fail on second invocation of run
4 participants