-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
For SQL-based targets, add built-in handling for schema_mapping
#1086
Comments
@aaronsteers what do you think about adding this one to the milestone SQL Target Out-of-Box Stability |
@BuzzCutNorman Good idea! Otherwise (at least without |
In my comment here: I lean towards keeping with pipelinewise precedent:
|
default_target_schema
and schema_mapping
@aaronsteers @kgpayne I just ran an issue when I bummped my
I was able to override the WDTY? Good idea, ok idea not the best, or there be monsters!!! |
This is a great idea, and I think very similar to the approach we likely would have taken. We discussed in a prior community call (sorry, I forget the date) that supporting Then the logic would be:
The third bullet above is a bit more nebulous, and I don't think we necessarily have to solve that here. |
@aaronsteers I have placed an override / overload ??? of the property function Please take a look let me know what you think of it so far
|
@BuzzCutNorman is the error caused by conflicting with the default If that's the case, and this |
@edgarrmondragon Yes I believe that is exactly why the error occurs. This is the error. |
@edgarrmondragon I was able to get the public mssql correction in place. I also noticed I had the function doing more work than it needed to do. While testing I ran into a scenario where two streams were sent to the same table. Below is the updated function and the tap
tap select example:
|
@edgarrmondragon I was looking to see what I needed to do to convert this to work in the Would I need to add to below code to the
or looking at "BATCH" there are |
@BuzzCutNorman Short answer is unfortunately no 😕. The capabilities are not tied to implementations of any kind, so taps or targets could report supporting them while they really don't and viceversa. We've been trying to figure out the best way to tie advertised capabilities to behavior.
Yeah, that's something we should try to prevent. I wonder if the wise targets handle this scenario or they have the same issue. It might make sense as a default behavior in case users want to merge streams coming from different source schemas. Either way it's probably a good test case for all SDK-based targets 🙂 |
Just to add a couple notes here... Re: declaration of the capability
That is correct. The SDK doesn't actually support it yet - but (to confirm) that is what this feature request would be delivering. Currently the feature is defined as:
And post-completion, we may want to expand upon the definition a bit more:
**Re: overriding capabilities definitions for I think you can declare an override for the For example of how to declare this, the base class has the following: Lines 127 to 140 in 75965be
Re: declaration of the config values: Similarly, I think you can override the Lines 303 to 331 in 75965be
Re: dealing with multiple streams loading to the same target table
I've run into this as well on other taps in cases where multiple databases can be extracted from at once, and when two databases both have a table with the same table and schema name. This is incredibly hard to detect and debug if the records from both streams are allowed to insert into the same table. The only way I know of to resolve this is to keep a dictionary of all of the "seen" pairings of target table name and source
Note: While I do think the above would be a good default behavior, as of now I don't know where this would best live - since the name resolution operation exists within the individual Hope this helps! Thanks! |
Thanks for that, I was starting to worry I missed something and all I needed to do was find the TODO and follow the directions. 😮💨 So super simplistic summary Thank you for the explanation of ways you can add a built-in capability config. 🙏😊 |
@aaronsteers , @edgarrmondragon @kgpayne I just put in a draft PR for this feature. When you get a chance, please give it a look. PR #1157 |
default_target_schema
and schema_mapping
schema_mapping
As discussed in:
The text was updated successfully, but these errors were encountered: