You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
One thought when I saw this though was that I worked on similar improvements in the SDK (re: meltano/sdk#1779), not sure if its exactly the same or not. Maybe you've already done this recently but you might want to compare what youre doing in the overridden methods here to make sure its not already done in the SDK for you. Its been a while since this target was created and the SDK has come a long way. Overridden methods wont get to take advantage of ongoing improvements so removing them if theyre no longer needed could be a beneficial.
I gave it a couple hours and wasn't able to get a reasonable implementation with the SDK's new prepare_table(). Many of the methods in target-postgres are overridden and have different expectations for input parameters than what is found in the SDK, meaning that it would take a lot to "unwind" the current target-postgres implementation (not as simple as just removing the prepare_table() override).
Based just on comparing the two codebases visually, I expect that the SDK does not address the same problems as #228. Following the path of execution, it's still O([number of columns]) db queries to prepare a table. Caching would speed things up after the initial run, but it would still fundamentally requires a large number of queries to set up.
One thought when I saw this though was that I worked on similar improvements in the SDK (re: meltano/sdk#1779), not sure if its exactly the same or not. Maybe you've already done this recently but you might want to compare what youre doing in the overridden methods here to make sure its not already done in the SDK for you. Its been a while since this target was created and the SDK has come a long way. Overridden methods wont get to take advantage of ongoing improvements so removing them if theyre no longer needed could be a beneficial.
Originally posted by @pnadolny13 in #228 (comment)
The text was updated successfully, but these errors were encountered: