[fix](dbt-doris): the commands dbt docs generate/serve fail due to wrong function signature#60779
Conversation
Removed unused methods related to catalog retrieval and filtering schemas from the Doris adapter implementation. Use default dbt-adapter functions
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Code Review Summary
This PR removes overridden get_catalog, _catalog_filter_schemas, and _catalog_filter_table methods from the Doris dbt adapter, deferring to the base adapter defaults. The motivation is correct — the overridden get_catalog signature is incompatible with dbt-core 1.10+.
However, I found the following issues:
Critical
- Bug: Missing
raisekeyword forCompilationError— The original code calleddbt.exceptions.raise_compiler_error(...), which is a function that internally raises the exception. The replacementCompilationError(...)merely constructs the exception object but never raises it. This means the validation check in_get_one_catalogis silently broken — iflen(schemas) != 1, execution will continue past the guard clause instead of aborting. The fix israise CompilationError(...).
Minor
- Unused import and variable —
AdapterLoggeris imported andlogger = AdapterLogger(__name__)is defined, butloggeris never used anywhere in this file. These should be removed to keep the code clean.
Checkpoints
- Correctness: FAIL — missing
raisekeyword is a bug that silently swallows an error condition. - Unused code: WARN —
loggerandAdapterLoggerimport are unused. - Removal of methods: OK — removing
get_catalog,_catalog_filter_schemas,_catalog_filter_tableis appropriate since the base adapter provides compatible defaults. - Import cleanup: OK — removed imports that were only used by the deleted methods.
| if len(schemas) != 1: | ||
| dbt.exceptions.raise_compiler_error( | ||
| CompilationError( | ||
| f"Expected only one schema in Doris _get_one_catalog, found " f"{schemas}" |
There was a problem hiding this comment.
Bug: Missing raise keyword. This line constructs a CompilationError object but does not raise it. The error condition will be silently ignored and execution will fall through to super()._get_one_catalog(...) even when len(schemas) != 1.
The original code used dbt.exceptions.raise_compiler_error(...) which is a function that raises internally. The replacement needs an explicit raise:
raise CompilationError(
f"Expected only one schema in Doris _get_one_catalog, found " f"{schemas}"
)| from dbt_common.exceptions import CompilationError | ||
|
|
||
| from dbt.adapters.events.logging import AdapterLogger | ||
|
|
There was a problem hiding this comment.
Nit: AdapterLogger is imported and logger is assigned (line 49) but never used anywhere in this file. Consider removing both the import and the logger variable to keep the code clean.
|
hi @Lastv25 PTAL at reviews |
|
/review |
There was a problem hiding this comment.
Code Review Summary
This PR removes custom get_catalog, _catalog_filter_schemas, and _catalog_filter_table methods from the Doris dbt adapter to rely on base adapter defaults, fixing a TypeError caused by a signature change in dbt-adapters.
Critical Checkpoints
Goal & Correctness: The goal is to fix dbt docs generate/serve commands. While removing the custom get_catalog is the right approach, the remaining _get_one_catalog override has an incompatible signature with the base class, which will cause a runtime failure. This is a blocking issue.
Modification Scope: The change is focused and minimal — only one file modified, removing overridden methods. Good.
Concurrency: Not applicable.
Lifecycle / Static Init: Not applicable.
Configuration Items: Not applicable.
Incompatible Changes: Yes — see critical issue below regarding _get_one_catalog signature mismatch.
Parallel Code Paths: Not applicable.
Test Coverage: PR mentions unit test, but no test changes are included in the diff. Given this is a bug fix removing overrides, manual testing of dbt docs generate should be sufficient.
Observability: Not applicable.
Performance: Not applicable.
Issues Found
-
[CRITICAL]
_get_one_catalogsignature mismatch — The remaining_get_one_catalogoverride usesmanifest: Manifestas the third parameter, but the base classBaseAdapter._get_one_catalogexpectsused_schemas: FrozenSet[Tuple[str, str]]. The baseget_catalog(which will now be called instead of the removed custom one) passesused_schemasto_get_one_catalog. This means aFrozenSetwill be passed whereManifestis expected, causing a runtime type error. The signature andsuper()call must be updated to match the base class, and theManifestimport should be removed (it becomes unused). -
[Minor] Extra blank line — There are 3 consecutive blank lines between the imports and the
Engineclass (lines 45-47). PEP 8 recommends exactly 2 blank lines between top-level definitions.
| information_schema: InformationSchema, | ||
| schemas: Set[str], | ||
| manifest: Manifest, | ||
| ) -> agate.Table: |
There was a problem hiding this comment.
CRITICAL: Signature mismatch with base class.
The base BaseAdapter._get_one_catalog signature (in dbt-adapters) is:
def _get_one_catalog(
self,
information_schema: InformationSchema,
schemas: Set[str],
used_schemas: FrozenSet[Tuple[str, str]],
) -> "agate.Table":But this override uses manifest: Manifest as the third parameter. Since the custom get_catalog was removed, the base get_catalog will now be called, which passes used_schemas (a FrozenSet[Tuple[str, str]]) as the third argument. This will fail at runtime because this override expects a Manifest object.
Fix: Update the signature to match the base class:
def _get_one_catalog(
self,
information_schema: InformationSchema,
schemas: Set[str],
used_schemas: FrozenSet[Tuple[str, str]],
) -> agate.Table:
if len(schemas) != 1:
raise CompilationError(
f"Expected only one schema in Doris _get_one_catalog, found {schemas}"
)
return super()._get_one_catalog(information_schema, schemas, used_schemas)Also:
- Add
FrozenSetto the typing imports - Remove the now-unused
from dbt.contracts.graph.manifest import Manifestimport
| from dbt.adapters.sql.impl import LIST_RELATIONS_MACRO_NAME, LIST_SCHEMAS_MACRO_NAME | ||
| from dbt_common.clients.agate_helper import table_from_rows | ||
| from dbt.contracts.graph.manifest import Manifest | ||
| from dbt.adapters.contracts.relation import RelationType |
There was a problem hiding this comment.
Minor: Since _get_one_catalog signature should be updated to use used_schemas: FrozenSet[Tuple[str, str]] instead of manifest: Manifest, this import will become unused and should be removed.
Removed methods related to catalog retrieval and filtering schemas from the Doris adapter implementation. Use default dbt-adapter functions.
What problem does this PR solve?
Problem Summary:
The
dbt docs ...commands do not work with the dbt-doris adapter due to the errorTypeError: DorisAdapter.get_catalog() takes 2 positional arguments but 3 were given. The signature of the get_catalog function changed in this commit dbt-labs/dbt-adapters@5d90ff9 . There is no need to redifine the get_catalog function in the adapter (same with _catalog_filter_table and _catalog_filter_schema functions), the default ones work.Solution:
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)