Make functionNullability property consistent in ResolvedFunction

[weiatwork]

The mismatch of "nullability" and "functionNullability" could cause failure of JSON deserialization for ResolvedFunction.

The symptom is query that involves function usages fails with error message like:
java.lang.IllegalArgumentException: Invalid JSON bytes for [simple type, class io.trino.metadata.ResolvedFunction]

Root cause is #10183, and specifically 2c1ebd6

The problem is exposed starting from #11217

Repro steps:

1. Create a file-based access control policy for a table with row filter or column mask that uses a function "filter": "name LIKE 'A%'"
2. Run a query against the table

Description

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

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

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

Related issues, pull requests, and links

Documentation

(x) 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

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

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

[findepi]

The change makes sense. Seems ResolvedFunction wants to be serializable but we don't exercise that code in any way.
@weiatwork did you try to expose the problem with a regression test?

[weiatwork]

Thanks for taking a look. I just happened to realize my query started failing with functions in AC policies. I couldn't reproduce with pure query using functions though.

[findepi]

Could you please try to repro the problem with a test in TestFileBasedSystemAccessControl?

[weiatwork]

There seems to be already test case that covers this (using functions). But I'm guessing it is not easily reproducible if not in an end-to-end manner.
https://github.com/trinodb/trino/blob/master/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java#L1350

[findepi]

@weiatwork a test can be as end-to-end as needed to exercise given portion of the code.
it seems to me that we rely on ResolvedFunction being json-serializable, but it didn't work, which points at a gap in our testing.

[weiatwork]

Good point. I agree this should have been covered in the UT. @wendigo Do you have any idea how we can get this tested out? Thanks.

[weiatwork]

@dain @wendigo In case you missed the update

[wendigo]

@weiatwork as @findepi said we need to add a test in TestFileBasedSystemAccessControl which uses a row filter function that you've mentioned. See for a reference: core/trino-main/src/test/resources/catalog_read_only.json which is used in testCatalogOperationsReadOnly() test method

[wendigo]

@weiatwork @findepi there is already a round-trip test in TestResolvedFunction

[weiatwork]

@wendigo Thanks for the pointer. It's interesting that when I run TestResolvedFunction locally without my fix, this test does fail, and with my fix it doesn't. Why doesn't CI have the problem? Is this test ever run by CI?

[wendigo]

@weiatwork actually for me this tests passed locally, without your change

[weiatwork]

I just rebased my code to latest, and yes I also got the test succeed even without my change. That's strange. Anyway, I think the fix is still needed for sanity reason. But looks like this PR is marked as "merged" (possibly due to my rebasing of my master branch). @findepi Can you help with this PR?

[findepi]

@weiatwork Can you please open a new PR?

[weiatwork]

@findepi #13777 has been created for the same. Sorry for the inconvenience

[findepi]

when I run TestResolvedFunction locally without my fix, this test does fail

it doesn't fail for me