Warn when SQL function parameter name shadows a column in a referenced model

[nitrajen]

Resolves #12707

Problem

PostgreSQL SQL-language functions silently resolve a parameter name to a same-named column when both are in scope. Consider a function with a customer_id parameter that queries a model which also has a customer_id column:

CREATE FUNCTION get_order_total(customer_id integer)
RETURNS numeric AS $$
    SELECT sum(amount)
    FROM {{ ref('orders') }}
    WHERE customer_id = customer_id
$$ LANGUAGE sql;

Both sides of WHERE customer_id = customer_id resolve to the column. The function runs without error but silently returns wrong results.

Solution

Added a check in FunctionRunner.execute(). At execution time, dbt has already run any models the function depends on. The check queries the database for their columns and warns if any parameter name matches. The check runs only for PostgreSQL. The warning is non-fatal and respects --warn-error.

Also depends on: companion PR to dbt-labs/dbt-adapters to enable this warning when parameter and column names conflict in PostgreSQL: dbt-labs/dbt-adapters#1828

Depends on a dbt-protos update to add the following message:

message FunctionParameterColumnConflict {
  string function_name = 1;
  string param_name = 2;
  string column_name = 3;
  string model_name = 4;
}

To test locally, I updated the serialized descriptor blob in core_types_pb2.py and appended the following type stubs to core_types_pb2.pyi:

@typing.final
class FunctionParameterColumnConflict(google.protobuf.message.Message):
    """I078"""
    function_name: builtins.str
    param_name: builtins.str
    column_name: builtins.str
    model_name: builtins.str

@typing.final
class FunctionParameterColumnConflictMsg(google.protobuf.message.Message):
    @property
    def info(self) -> Global___CoreEventInfo: ...
    @property
    def data(self) -> Global___FunctionParameterColumnConflict: ...

How the change was tested

Added a functional test in tests/functional/functions/test_udfs.py that verifies warnings fire for shadowed parameters and not for clean ones.
Also ran dbt build locally against PostgreSQL with two clean functions and two shadowed functions to confirm the same behavior end to end.

Checklist

• I have read the contributing guide and understand what's expected of me.
• I have run this code in development, and it appears to resolve the stated issue.
• This PR includes tests, or tests are not required or relevant for this PR.
• This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
• This PR includes type annotations for new and modified functions.

[cla-bot]

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @nitrajen

[nitrajen]

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @nitrajen

I've signed the CLA

[nitrajen]

Some CI checks I assume will fail because of changes to the dbt-protos lib which appears bot-managed. I've described the changed in the PR description. If someone from the dbt team advises me what to do, I'd be willing to do it. The CLA CI check keeps coming up as failed. I did get a email and I signed it - twice.

[codecov]

Codecov Report

❌ Patch coverage is 40.00000% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.78%. Comparing base (1076481) to head (1baea41).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12772      +/-   ##
==========================================
- Coverage   91.41%   83.78%   -7.63%     
==========================================
  Files         203      203              
  Lines       25883    25906      +23     
==========================================
- Hits        23660    21705    -1955     
- Misses       2223     4201    +1978     

Flag	Coverage Δ
integration	83.78% <40.00%> (-4.50%)	⬇️
unit	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
Unit Tests	83.78% <40.00%> (-7.63%)	⬇️
Integration Tests	83.78% <40.00%> (-4.50%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nitrajen]

I opted for a functional test in lieu of adding unit tests because the change was only a few lines, and I thought it might be slightly better to have functional tests and not unit tests, because the existing unit tests covered udfs to a decent degree. Any reviewer who picks this up, let me know. Totally willing to add unit tests if you think they are needed.

[nitrajen]

@jtcohen6 @drewbanin - just tagging you in since I've seen your participation in a number of issues. Is this PR and tagged issue worth looking at? Would love any thoughts.