Restore removed (but used) methods in common.sql

[potiuk]

Unfortunately two of the methods that have been removed in #26761 have been used in Google Provider 8.4.0

Those methods should be restored and 1.3.0 version of the common.sql has to be yanked to get rid of the problem when Bigquery is broken by common.sql

Fixes: #27838

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[potiuk]

cc: @denimalpaca

[potiuk]

Just for the future - (note to self as well) we should be REALLY careful when removing code from common providers.

[eladkal]

Keep untill when? What is the condition that it will be safe to remove

[potiuk]

Good question. I think there is no obvious removal condition. I could not find a good one. 8.4.0 can be kept for a long time and and one can continue upgrading common sql. you can easily install 8.4.0 on Airflow 2.8 and with comon.sql 1.10 for example - theorethically should be no problem. We have no rules for deprecating / EOL of providers. themselves.

[potiuk]

This is one of the problems with such a common code. You HAVE TO keep it as backwards compatible as possible - because it is shared between multiple packages., You can very easily imagine someone sticking with google provider 8.4.0 and upgrading some other providers that are using common.sql

This is somethign I mentioned at the beginning of our common.sql journey - once we released something there, it should stay there basically forever (unless we introduce EOL policies for providers, which we do not have currently).

[eladkal]

So why not releasing 8.4.1 with limiting the sql provider. It wont solve the issue but it can help users to quickly migrate out of 8.4.0 as nothing is changed other than pinning upper version which is also the version they are currently using

[potiuk]

Because it has been already installed and running for over a month and upgrading conmon.sql provider will break it now. and the worst thing is that by upgrading another package you break google one. This is quite unexpected for those users and will create a flurry of problems if we do not act quickly.

I think it's a small price to keep 20 lines of code.

[eladkal]

We need this PR anyway so lets merge it and prepare the first step. we can discuss additional steps in parallel/after

[dstandish]

rather than adding the code back, it might be better to instead just add it to the google provider and make a patch release for that.

if ultimately we don't want it in core, seems best to keep it out. wdyt?

[eladkal]

@dstandish that will not solve the issue.
The issue is that google provider 8.4.0 which was released a while ago had no upper limit for sql provider. So once we release new version of sql provider we are breaking all installs with 8.4.0

There is no issue for people who updated google provider for new version. Version 8.5.0 works fine with the new sql provider release.

[dstandish]

oh, yeah

[potiuk]

Yes. The big problem is that the cat is out of the bag now. And just upgrading to currently released 1.3.0 of common.sql will break already released 8.4.0 - in a bad way. Releasing 1.3.1 is just prevention of (VERY) likely problems to happen.

1. release 1.3.1 QUICKLY and yank 1.3.0 (the coming wave of provider fixes) - result - no matter what combo of common.sql + google provider user has, it will work. This is plain, tactical solution to prevent flood of issues to handle and big inconvenience for our users. I'd call it "preventive hotfix".

2. reease 8.4.1 with a fix as next step.

3. decide what will be the condition to remove the added code in common.sql

[denimalpaca]

@potiuk I'm not entirely following why this broke (would like to know so I don't do it again in the future!); when I removed the method, I added the code back in to the BigQuery version of the operators here. But that doesn't seem to be enough? Do certain versions just not have this change available? And what checks would ensure that versions across providers will match when updates like this happen? (i.e. should I have made the BigQuery changes in a Google providers specific PR?)

[potiuk]

@potiuk I'm not entirely following why this broke (would like to know so I don't do it again in the future!); when I removed the method, I added the code back in to the BigQuery version of the operators here. But that doesn't seem to be enough? Do certain versions just not have this change available? And what checks would ensure that versions across providers will match when updates like this happen? (i.e. should I have made the BigQuery changes in a Google providers specific PR?)

The problem had several steps and I tink this is a good learning for us for such common code for things that we should:

a) look after during the review
b) add automated protection for in common providers

Again - this is something that we (or at least I) kind of expected when we started the common.sql journey. We picked common.sql as relatively small and one that we will be able to fix rather quickly if problems happen, but it had already shown us a number of scenarios that neither authors of changes, nor reviewers were aware of.

"Common code" in separate providers is not an easy one, has multiple traps and unobvious problems. We should not be treating "separate comon code" lightly without realising it might have issues like that - mainly because the code is both separate and coupled. The problems starts where coupling is unintended and not obvious (this was this case).

I treat that as a very good learning so let me share the sequence of events (again @denimalpaca - do not treat it as something that you are "guilty" of - it happened that you were bold enough to make the changes, but you are as guilty as we as reviewers who failed to spot the problem).

Sequence of events:

1. Change adding columns in Google Provider Add BigQuery Column and Table Check Operators #26368:

The added code that caused the problem:

This change adds coupling between BigQuery part of google provider and common.sql's in an unintended way. It is using both _get_failed_checks, and parse_boolean which were not really intended to be part of the API.

2. Google provider 8.4.0 has been releaased with those imports. That made it "official". At the moment google provider 8.4.0 has been released, those two methods became "API" - because they had a user (google provider 8.4.0) - and that user expects the methods to be there in the future version of 1.* version of common.sql (backwards compatibility)

3. The Common sql bugfixes and improvements #26761 removed both methods from common.sql

4. We released common.sql 1.3

Now everyone who has 8.4.0 of Google Provider (many users) and upgrades common.sql to 1.3 (for example because they upgrade any other provider that has dependency on 1.3 new features) will break google provider.. Thy can fix it by upgrading google provider to 8.5.0, yes, but still the effect will be that by upgrading (for example) Presto, thy will break google BigQuery. We want to avoid that situation to escalate.

My learnings for the future:

1. pay more attention during development and review and make sure we only use stuff that is intended to be backwards compatible (part of the API)

2. pay more attention when removing code to see if there are any users of even the things that we think are not part of the API

3. automate check for API contracts - i.e. implement automated checks that will catch and detect mis-use of non-public "API" of comon code (we already have some of that for a number of "core" features actually - one of those checks is https://github.com/apache/airflow/blob/main/scripts/ci/pre_commit/pre_commit_check_provider_airflow_compatibility.py - we need to have similar for "common" part )

This is actually Hyrum's law in full swing (only we did it ourselves with our own internal user - google provider):

With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody.

In this case both _get_failed_checks, and parse_boolean were not supposed to be depended on, but unfortunately, they were.

[luixi79]

Is there an ETA to officially release new version 1.3.1 and get rid of 1.3.0? All my pipelines in Dev are broken until "_get_failed_checks" is recovered. Thanks!

[denimalpaca]

@potiuk thanks for the explanation, that's very clear to me now. Don't worry, I'm not too riddled with guilt over this -- just very intent on making sure I learn from mistakes!

[potiuk]

Is there an ETA to officially release new version 1.3.1 and get rid of 1.3.0? All my pipelines in Dev are broken until "_get_failed_checks" is recovered. Thanks!

We are in the process of voting and testing the releae now. And it is governed by the https://www.apache.org/foundation/voting.html#ReleaseVotes and current release are in this discussion on our devlist https://lists.apache.org/thread/cvzkqhdqhwhyb3gp81nzs9zcl3hjwxnz

You can downgrade to 1.2.0 as workaround.

But also if If you want to help with testing and making us more confident with the release I heartily invite you @luixi79 to test RC3 candidates - all details are in #27939 . You will find information about the RC3 candidates and by following the PyPI links you should be able to install the RC.

And I also encourage you to report the status, whether your problem is fixed and generally helping in raising confidence that the release is good. Responding on the devlist is also cool - stating for example that it fixes your problem and cannot wait for it.

Voting process of Apache Software Foundation lasts for 72 hours and this period ends tomorrow mid-day CET. But whether we will have enough votes by that time or wait longer also depends on the confidence we have from people like you testing the RCs and telling us "yeah, it works, I tested it - it solves my problem".

So when you are asking me for ETA, my response is - the more you (and users like you) help with building confidence the faster it happens - but not sooner than tomorrow mid-day CET.