Revert "Improve Redshift"

[findepi]

Reverts #15365
That PR was merged violating the code of conduct (lack of any form of attribution to actual code authors), Trino review process (quick merge without letting other maintainers to chime in) and jeopardizing the quality (lack of automated tests).
In the spirit of @martint's #15403, we should revert the change and re-submit it again. This is necessary to unblock the release.

[martint]

@findepi , have you identified any technical issues with this PR? Can they be addressed incrementally?

Regarding testing, I don't believe we have any automated tests for redshift at this time, so that PR doesn't change this fact. However, it appears based on the comment in the PR that tests were run on the new code:

I have run the test manually and they all pass.

Unless there's something broken (a regression) or this is introducing user-facing behaviors that need more discussion, I don't think there's a point in reverting it, especially if we're just going to turn around and resubmit the same code.

[kokosing]

this is introducing user-facing behaviors that need more discussion,

#15365 is changing type mapping of the Redshift connector. Any production usage of this connector will be now broken after the update of Trino. There is no way to restore previous behavior.

[kokosing]

Unless there's something broken (a regression) or this is introducing user-facing behaviors that need more discussion, I don't think there's a point in reverting it, especially if we're just going to turn around and resubmit the same code.

Please take a look at the PR description. To me, the motivation behind this PR is not that much technical but it is more about the process it was done. I would prefer to avoid such precedences to avoid situations in future where one is using #15365 as justification for shortcuts.

[martint]

We can talk about process and improve it going forward, but I don’t see how reverting the change fixes that.

If the type mappings are a problem, we should fix those or revert that part (assuming we don’t want that behavior at all). Is that what we want to do?

[findepi]

Can they be addressed incrementally?

Things that cannot be fixed incrementally:

• attribution to actual code authors
• shipping new untested code in a release
• setting bad precedence for testing infrastructure for new plugins like Snowflake connector
  • lack of testing is the main reason why Add Snowflake JDBC Connector #2551 and Add Snowflake JDBC Connector #10387 (and maybe others such) haven't been merged yet. We don't want to give impression that some community members are not allowed to contribute untested code, while some others are.
• review process of a change that bypassed review and was already merged

[kokosing]

If the type mappings are a problem, we should fix those or revert that part (assuming we don’t want that behavior at all). Is that what we want to do?

I am not sure if it is what we want. I think instead of changing the existing connector I would prefer to add a new one and deprecate the old one. Then eventually remove the old one.

[martint]

attribution to actual code authors

Reverting doesn't fix that either. The authors were added to the original PR description: "This was co-authored by: @alexjo2144 @ebyhr @findepi @findinpath @grantatspothero @hashhar @jirassimok @kokosing @losipiuk @Praveen2112 @raunaqmorarka @skrzypo987 @ssheikin @wendigo"

shipping new untested code in a release

It was tested manually, according to the PR description. There's currently no infrastructure for testing the Redshift connector automatically, so that's all we can do at this time.

review process of a change that bypassed review and was already merged

As far as I can tell, it was reviewed by @electrum (and, indirectly, @dain, since he wasn't one of the original authors). However, unless there's something broken or fundamentally wrong with the changes, applying incremental fixes is preferable at this point.

[findepi]

(just rebased, due to a force-push to master (#15365 (comment)), no other changes)

[hashhar]

We have a precedent of not accepting connectors without tests running in CI (excluding things which we inherited from Presto), preserving attribution/CLA when contributing code and doing transparent code reviews - regardless of the source or author of the PR.

So the action IMO should be:

• Merge the revert
• Rename existing connector to be redshift-legacy to indicate it's actual nature
• See if someone is willing to contribute a Redshift testing environment
• If yes, then re-submit the PR with proper history and attribution maintained.

Since the people maintaining the JDBC connectors is mostly me, @ebyhr and @findepi I'll wait for @ebyhr's opinion and merge this then.

Also the connector changes are a breaking change because queries now return different results and any CTAS/INSERT queries which people had will start failing until people change the table definition. Both versions 403 and 405 will not be able to co-exist and it'll be a one-way upgrade.

[martint]

I still haven’t heard a compelling technical reason for reverting this change. If there are parts that are problematic, then we should fix them or revert those specific parts. From what I see in the list of commits, there are at least a few we want to keep around.

[martint]

In particular, the only problematic one seems the one that changes the type mappings, but the other ones should not break compatibility and are good improvements to the connector.

[ebyhr]

@martint What's your opinion about "setting bad precedence for testing infrastructure for new plugins like Snowflake connector..." in #15410 (comment)?

[hashhar]

I'm not saying the changes are bad.
This revert doesn't mean they can't be re-submit in a new PR that follows the bar for contributions we already follow.

[martint]

@ebyhr My opinion on that, and I know it’s not shared by many others, is that not all code can be easily tested and testing is inherently flawed anyway, so that should not be an impediment to making forward progress.

As long as good faith effort was made to test a change (manually, by adding manual tests or automated tests), I would have no problem accepting a change. Testing provides degrees of confidence on the correctness of a change, but they cannot prove it works in all situations, existing and future ones.

[findepi]

is that not all code can be easily tested and testing is inherently flawed anyway

agreed 💯
fortunately redshift connector doesn't fall into this category.
It doesn't cost a ton to test with real redshift instances, so if Starburst wants to contribute a Redshift connector (the PR being reverted was raised by a Starburst employee), we can ask them to cover the CI expenses too. Trino project is not a stove-away for someone's code, it's something we cherish and can trust, because it's proven & tested. Continuously, not one-off.

[martint]

@kokosing @hashhar, this PR adds a legacy flag for the old type mapping rules to allow users to migrate at their own pace: #15513

[findepi]

The revamped Redshift plugin has been released, so we no longer have a door open to reintroducing it respecting the process (allow review) and quality (testing what we ship). It is good that the code authorship has been restored.

The ship has sailed.