fix(tableau): skip populate_views for filtered workbooks; O(1) project lookup - #33584
zerafachris wants to merge 1 commit into
Conversation
…t lookup Large Tableau instances call `populate_views(workbook, usage=True)` for every workbook before any dashboard/project filtering, making full syncs take days. Changes: - `get_all_projects()`: build `all_projects` as a `dict[str, ProjectItem]` keyed by project id for O(1) lookup (was O(n) linear scan via `next()`). - `get_workbooks()`: accept an optional `filter_fn(name, project_path) -> bool` callback evaluated *before* the expensive `populate_views` API call. Workbooks that match the filter are skipped entirely. - `get_dashboards_list()`: build an `_early_filter` closure from `dashboardFilterPattern` / `projectFilterPattern` and pass it to the client, so discarded workbooks never pay the `populate_views` cost. Fixes open-metadata#33441 Prepared with AI assistance (Claude Code, Anthropic), reviewed for correctness before submission. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| def _early_filter(name: str, project_path: str | None) -> bool: | ||
| """Return True if the workbook should be skipped before populate_views.""" | ||
| return filter_by_dashboard(dashboard_pattern, name) or filter_by_project( | ||
| project_pattern, project_path | ||
| ) | ||
|
|
||
| yield from self.client.get_workbooks( | ||
| include_owners=self.source_config.includeOwners, | ||
| filter_fn=_early_filter, | ||
| ) |
There was a problem hiding this comment.
⚠️ Bug: Early project filter lacks single-name fallback, drops workbooks
The downstream project filter in dashboard_service.get_dashboard() computes project_name from get_project_name() (the workbook's immediate project.name) and only overrides it with the dotted parent path when get_project_names() returns a non-empty value. The new _early_filter instead passes only project_path (the result of get_project_parents_by_id) to filter_by_project, with no fallback. When path resolution returns None — e.g. get_all_projects() raised and left all_projects empty, or the workbook's project is missing from the fetched dict — filter_by_project(pattern, None) returns True for any configured pattern (see filters.py line 58-60), so the workbook is skipped early. The original pipeline would have kept it by falling back to the immediate project name. This silently drops dashboards precisely in the filtered large-deployment case this PR targets.
Only apply the project filter early when the parent path actually resolved, matching the downstream fallback so no workbook is wrongly skipped.:
def _early_filter(name: str, project_path: str | None) -> bool:
"""Return True if the workbook should be skipped before populate_views."""
if filter_by_dashboard(dashboard_pattern, name):
return True
# Mirror dashboard_service: only skip on project when the path resolved.
# A None path downstream falls back to the immediate project name, so
# skipping here would drop workbooks that would otherwise be kept.
if project_path is not None and filter_by_project(project_pattern, project_path):
return True
return False
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| project_path = self.get_project_parents_by_id(str(workbook.project_id)) | ||
| if filter_fn(workbook.name, project_path): | ||
| continue |
There was a problem hiding this comment.
If project discovery fails or a workbook references a project missing from all_projects, get_project_parents_by_id returns None. With an enabled project filter, that missing value is treated as filtered and the workbook is skipped. The previous downstream filter instead fell back to workbook.project_name, so a transient or incomplete project listing can now silently exclude workbooks that should be ingested.
Knowledge Base Used:
| def _early_filter(name: str, project_path: str | None) -> bool: | ||
| """Return True if the workbook should be skipped before populate_views.""" | ||
| return filter_by_dashboard(dashboard_pattern, name) or filter_by_project( | ||
| project_pattern, project_path | ||
| ) | ||
|
|
||
| yield from self.client.get_workbooks( | ||
| include_owners=self.source_config.includeOwners, | ||
| filter_fn=_early_filter, | ||
| ) |
There was a problem hiding this comment.
Workbooks rejected by this early filter never reach the existing status.filter(...) calls in the downstream dashboard stage. Ingestion summaries therefore no longer count or identify dashboards rejected by dashboard or project patterns, making it harder for operators to understand why records were omitted.
Knowledge Base Used:
| self.all_projects = { | ||
| str(project.id): project for project in Pager(self.tableau_server.projects) | ||
| } |
There was a problem hiding this comment.
This dictionary eagerly retains every Tableau project without an explicit size limit. That violates the repository directive that all caches must be bounded and that bare dictionaries must not be used as unbounded caches. This requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Fixes #33441 — on large Tableau instances the connector calls
populate_views(workbook, usage=True)for every workbook before any dashboard/project filtering is applied. This makes full syncs take days.Changes
ingestion/src/metadata/ingestion/source/dashboard/tableau/client.pyget_all_projects()now buildsall_projectsas adict[str, ProjectItem]keyed bystr(project.id). The previous implementation stored a flat list, soget_project_parents_by_idhad to do an O(n)next(… for … in self.all_projects …)scan on every recursive call. The dict makes each lookup O(1).get_workbooks()gains an optionalfilter_fn(name: str, project_path: str | None) -> boolparameter. When provided, the closure is evaluated before the expensivepopulate_viewsAPI call. ReturningTrueskips the workbook entirely, so filtered-out workbooks pay no cost.ingestion/src/metadata/ingestion/source/dashboard/tableau/metadata.pyget_dashboards_list()builds an_early_filterclosure from the existingdashboardFilterPattern/projectFilterPatternsource-config knobs (using the existingfilter_by_dashboard/filter_by_projecthelpers) and passes it asfilter_fnto the client.Impact
No functional change for instances without filter patterns. For instances that do filter (the common case for large deployments), workbooks that would previously be discarded after
populate_viewsare now discarded before it, making the sync proportionally faster.Tests
test_tableau_client.pytests pass.test_tableau.pytests pass.ruff checkis clean on both changed files.Prepared with AI assistance (Claude Code, Anthropic), reviewed for correctness before submission.
🤖 Generated with Claude Code
The PR is not safe to merge until missing project paths preserve the existing direct-project fallback and the explicit bounded-cache requirement is satisfied.
Findings
Summary
This PR adds pre-fetch Tableau workbook filtering and replaces linear project lookup with an indexed lookup.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR C[Dashboard and project patterns] --> E[Early workbook filter] P[Project ID index] --> E E -->|Keep| V[Populate views and datasources] V --> D[Downstream dashboard stage] D --> O[Emit dashboard metadata] E -->|Skip| X[No API enrichment] X --> N[No downstream filtered-status record]Reviews (1) · Last reviewed commit: "fix(tableau): skip populate_views for fi..."