Skip to content

dbt-materialize: report clear errors on the adapter's error paths - #38070

Merged
bobbyiliev merged 1 commit into
MaterializeInc:mainfrom
bobbyiliev:dbt-adapter-error-paths
Aug 27, 2026
Merged

dbt-materialize: report clear errors on the adapter's error paths#38070
bobbyiliev merged 1 commit into
MaterializeInc:mainfrom
bobbyiliev:dbt-adapter-error-paths

Conversation

@bobbyiliev

Copy link
Copy Markdown
Contributor

A handful of error paths in the adapter did not do what they looked like they did, so users hit confusing failures instead of the messages we wrote for them. This fixes the index config parser, the incremental and listagg explanations, the branch in rename_relation that read an undefined variable, the missing fallback in drop_relation, backslash and non-string handling in connection options, and the version check against a server that is not Materialize.

Test plan: added test_relation_macros.py for the index config error, renaming a view and dropping an unsupported relation type, plus a case in test_incremental.py that checks the explanation is what users actually see.

@bobbyiliev
bobbyiliev force-pushed the dbt-adapter-error-paths branch 8 times, most recently from 1759db8 to dd97151 Compare August 19, 2026 13:59
@bobbyiliev
bobbyiliev marked this pull request as ready for review August 19, 2026 14:13
@bobbyiliev
bobbyiliev requested a review from a team as a code owner August 19, 2026 14:13
@bobbyiliev
bobbyiliev requested a review from jubrad August 19, 2026 14:13

@jubrad jubrad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Six real fixes, all of them doing what the changelog says; I verified the backslash escaping against pgwire's own split_options and the new drop_relation error against what the relation cache can actually produce, and both hold up.

Summary

2 considerations

written by claude on behalf of @jubrad

f" Got: {raw_index}\n"
' Expected a dictionary with at minimum a "columns" key'
)
raise IndexConfigNotDictError(raw_index)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considerIndexConfigNotDictError looks unreachable, and the tests agree with that reading: cls.validate reports non-dict input as a ValidationError, so parse("a") lands in IndexConfigError, and test_not_dict_error_message has to construct the exception directly to exercise its message.

Since the class exists only to be caught by a branch nothing takes, consider dropping the except TypeError arm and the class with it, and keeping just the ValidationError path. That also removes a message users can never see, which is otherwise the kind of thing that rots. (RefreshIntervalConfigNotDictError is presumably in the same boat, but that one is pre-existing and out of scope here.)

written by claude on behalf of @jubrad

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, dropped the except TypeError arm, the class, and the test that had to construct it directly.

assert "options" in credentials._connection_keys()


def test_backslashes_are_escaped():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider — These two assertions restate the implementation rather than test it: they check that \ becomes \\, which is exactly the line of code above them. What makes the fix correct is the parser on the other end, so the escaping order is only really proven by a round trip.

tests/adapter/test_connection_options.py already has TestConnectionOptionsOverrideEscapeSpaces doing precisely this for spaces. A sibling class with application_name: "app\\with\\backslashes" asserting current_setting('application_name') comes back unchanged would catch a future regression in the escaping order, which these unit tests would happily continue to pass through.

written by claude on behalf of @jubrad

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, replaced them with TestConnectionOptionsOverrideEscapeBackslashes in test_connection_options.py, mirroring the spaces sibling. The value is app\with\ backslashes, so it includes a backslash directly before a space, which pins the escaping order through a real round trip rather than restating the replace calls. Kept test_non_string_values_are_stringified since that one covers the AttributeError fix rather than the escaping line.

@bobbyiliev
bobbyiliev force-pushed the dbt-adapter-error-paths branch from dd97151 to d92738a Compare August 26, 2026 17:44
@bobbyiliev
bobbyiliev merged commit a66a9f5 into MaterializeInc:main Aug 27, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants