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

temp workaround for false approximate match #12

Merged
merged 6 commits into from
Nov 17, 2021
Merged

Conversation

dataders
Copy link
Contributor

@dataders dataders commented Nov 16, 2021

fixes: #11

#8 might be closer to the permanent solution, but I'm still bottoming out on root cause and will likely need to meet w/ @jtcohen6 sometime next week.

this isn't a permanent fix, but it will work in the short-term, while we dive deep on the source of the adapter.get_relation issue

an overly-simplified normal operation for the table materialization is:

  1. get the database, schema and identifier of the relation we are trying to make
  2. go look and see if a relation with the same three-part name already exists and assign result to old_relation
    1. if yes, return an Relation object with the three-part name, and a type property (so we know if it exists as a view or as a table)
    2. if no, return None
  3. if old_relation is not None, drop the pre-existing relation.

however, because we currently do not support views, if we assume that there are no views currently in the database, then we don't need to step 2 above. instead we can always just call DROP TABLE <TABLE_NAME> IF EXISTS, because:

  1. the relation should always exist, and
  2. if the relation doesn't already exist, the statement won't fail because of the ... IF EXISTS clause

@octavianzarzu30
Copy link
Contributor

verified it works.

@dataders dataders marked this pull request as ready for review November 17, 2021 00:31
@dataders dataders added this to the 0.21.3 milestone Nov 17, 2021
@dataders
Copy link
Contributor Author

@octavianzarzu30 if this looks good to you, I can release this change as 0.21.3 tomorrow.

@octavianzarzu30
Copy link
Contributor

yep looks good, have even tried it today :)

Copy link
Contributor

@ima-hima ima-hima left a comment

Choose a reason for hiding this comment

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

One formatting change, one documentation request.

@dataders dataders requested a review from ima-hima November 17, 2021 19:32
Copy link
Contributor

@ima-hima ima-hima left a comment

Choose a reason for hiding this comment

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

lgtm.

And thanks for the additional comment. It actually did help my conception.

@dataders dataders merged commit 47dfc52 into main Nov 17, 2021
@ima-hima ima-hima deleted the run_twice_workaround branch November 17, 2021 19:40
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.

adapter.get_relation mistakes exact match for approximate match
3 participants