SQL MERGE throw if assigning null to non-null column

[djsagain]

Description

SQL MERGE throw if assigning null to non-null column

This commit changes the two implementations of
MergeRowChangeProcessor to throw an exception if a
null value is assigned to a non-null column as a
result of an SQL MERGE operation. An alternative
approach would have been to change the query plan
to insert such checks, but the chosen approach is
simpler.

This commit adds a BaseConnectorTest showing that SQL MERGE
prevents assignment of NULL to non-null target table columns.

Is this change a fix, improvement, new feature, refactoring, or other?

It's either a fix or a new feature, depending on how you look at it. It only applies to SQL MERGE which was merged two days ago.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

These changes were to the machinery supporting SQL MERGE in the Trino engine.

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Fixes #13526

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[findepi]

are the build failures related?

[findepi]

As commented under the issue #13526 (comment), this should ideally be enforced by the planner-injected checks. You can see #13462 for how this can be done. Alternatively we can merge this PR and @homar or someone else will follow up.

[electrum]

Do we want to filter out hidden columns? What happens if someone assigns to a hidden column?

[djsagain]

I didn't know it was possible to assign to a hidden column.

I'll have to look at everywhere else where I've excluded hidden columns.

[djsagain]

Alternatively we can merge this PR and @homar or someone else will follow up.

I thought I'd responded to this but I don't see the response. Yes, since setting non-null target columns to null is a correctness issue, I think it's best if we merge this PR and then follow up with an alternative solution if we think it's beneficial.

[findepi]

I thought I'd responded to this but I don't see the response. Yes, since setting non-null target columns to null is a correctness issue,

it is

I think it's best if we merge this PR

it it solves the problem, yes

but mind that similar approach for inserts didn't actually work: #13434
so please make sure to test-cover all these cases.

[djsagain]

I really think this PR should be merged, since allowing null to be assigned to a non-null column is a big-time correctness issue. Can someone who agrees and has the ability to merge it please do so?

[martint]

It would be useful to indicate which column has the issue

[electrum]

We could add them to MergeParadigmAndTypes

[djsagain]

Added, and now the non-null column test lists the non-null column name assigned to.

[electrum]

We could add them to MergeParadigmAndTypes

[electrum]

Let's remove the hidden filter here. It seems unrelated to nullability. If we later allowed updating hidden columns (not unreasonable), it would be easy to miss updating the code here.

[djsagain]

Removed.

[djsagain]

I've updated the PR as suggested by @electrum and @martint, and all tests pass. This PR should be merged.