Conversation
* This comes with a requirement to drop python versions older than 3.10 so I've pulled those out.
* tox is still hitting errors with the passenv line we I haven't yet addressed. Coming back to that in a subsequent commit. Overall, testing needs to be revisited to make sure it's robust enough.
This commit is the first pass at upgrading to the latest version of the dbt core APIs for the adapter. Once this is complete and compiles cleanly, we'll do subsequent passes to knock out runtime and semantic errors that remain. As of this commit, `poetry run pytest` at least compiles and executes with most tests passing. More work is needed, however. See below. This includes the following major changes: * A number of classes and types have moved package locations. I've done my best to make sure the replacements selected are semantically equivalent, but I'm not positive that's the case. For this pass, I'm targeting a clean compile (with type checking enabled) to catch the majority of obvious mistakes but it deserves a second pass. * Some of the error/exception names have changed, and some have no obvious replacement. I haven't done a deep dive to understand if the error types effect different error handling logic or if they're just used for semantic differentiation. This also deserves a second pass. * The custom implementation of BaseRelation is temporarily commented out. See the larger comment below on this. I've also left this comment in a comment in case someone only looks at the code. * DecodableAdapter had an implementation of AdapterProtocol that doesn't seem to be required anymore. I've commented it out and left it in place because I can't find any documentation on this change from dbt. I suspect this will cause some kind of issue once everything is running. * Some methods of BaseAdapter are now allowed to simply `pass` rather than return NotImplementedException (which no longer exists). expand_column_types() is one place that got this treatment. * get_thread_connection() has moved from BaseAdapter to BaseAdapter#connections. * DecodableRelation is currently unused due to the dataclass issue described above. On the DecodableRelation / frozen dataclass issue: For some reason the base class is calling cls.from_dict(kwargs) in create() on our implementation of Relation. This fails with an AttributeError because Relation is marked as a frozen dataclass (see @DataClass def) which is required because BaseRelation is also frozen. This means the base class is trying to mutate something it knows must be immutable which makes no sense. There is a deeper issue here that needs to be worked out. That said, it's not clear we require a custom implementation of BaseRelation. I've temporarily updated many of the methods that were referencing Relation to use BaseRelation instead since they didn't appear to touch any specific members of Relation. It's all very strange.
(cherry picked from commit 498557b) Signed-off-by: Sharon Xie <rxie@decodable.co>
(cherry picked from commit 221d403a2e244aeb700ee2b8b395f4eca1ac79ed) Signed-off-by: Sharon Xie <rxie@decodable.co>
Contributor
|
Just FYI: I merged the PR for using schema v2 stuff yesterday. I'm looking to publish a pre-release of v2 of the adapter with that for a customer, and then I'll switch over to reconciling this PR on those changes and pick up where we left off. Appreciate the details commit messages. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please see the git commit messages for detailed changes and status.