Skip to content

Add cluster key tests - Snowflake#27608

Closed
harshach wants to merge 3 commits intomainfrom
add_snowflake_tests_cluster_key
Closed

Add cluster key tests - Snowflake#27608
harshach wants to merge 3 commits intomainfrom
add_snowflake_tests_cluster_key

Conversation

@harshach
Copy link
Copy Markdown
Collaborator

@harshach harshach commented Apr 22, 2026

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Bug fix:
    • Replaced recursive cluster key parsing with an iterative DFS implementation to prevent RecursionError and ingestion pod crashes.
    • Added explicit RecursionError handling in parse_column_name_from_expr as a defensive measure against deep expression trees.
  • Testing:
    • Added comprehensive unit tests in test_snowflake.py covering deep recursion scenarios, DFS order preservation, and error handling for pathologically complex cluster keys.

This will update automatically on new commits.

Copilot AI review requested due to automatic review settings April 22, 2026 06:02
@harshach harshach requested a review from a team as a code owner April 22, 2026 06:02
@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Apr 22, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens Snowflake ingestion stability by making cluster-key expression parsing robust to deeply nested CLUSTERING_KEY expressions that previously could trigger RecursionError and crash the ingestion process.

Changes:

  • Replace the recursive cluster-key token walker with an iterative DFS to avoid Python recursion limits.
  • Add explicit RecursionError handling in parse_column_name_from_expr to log a warning and return an empty list.
  • Add unit tests that simulate very deep cluster-key token trees and validate ordering + logging behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
ingestion/src/metadata/ingestion/source/database/snowflake/metadata.py Switches cluster-key identifier extraction to an explicit-stack DFS and adds a RecursionError fallback path with warning + safe empty result.
ingestion/tests/unit/topology/database/test_snowflake.py Adds regression tests covering deep nesting, identifier DFS order preservation, repeated parsing, and downstream RecursionError logging behavior.

Comment on lines +890 to +892
from sqlparse.sql import Identifier as _SqlparseIdentifier # noqa: E402


Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

There’s a mid-file import (from sqlparse.sql import Identifier as _SqlparseIdentifier) guarded by # noqa: E402. This will fight the repo’s Python formatting tooling (e.g., isort) and makes imports harder to reason about. Please move this import up with the other module imports (near the existing from sqlparse.sql import Function) and drop the E402 suppression.

Copilot uses AI. Check for mistakes.
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 22, 2026

Code Review ✅ Approved

Adds comprehensive unit tests for Snowflake cluster keys to ensure proper schema validation. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

🔴 Playwright Results — 1 failure(s), 15 flaky

✅ 3695 passed · ❌ 1 failed · 🟡 15 flaky · ⏭️ 89 skipped

Shard Passed Failed Flaky Skipped
🔴 Shard 1 478 1 2 4
🟡 Shard 2 654 0 2 7
🟡 Shard 3 663 0 3 1
🟡 Shard 4 645 0 3 27
🟡 Shard 5 610 0 1 42
🟡 Shard 6 645 0 4 8

Genuine Failures (failed on all attempts)

Pages/SearchIndexApplication.spec.ts › Search Index Application (shard 1)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoEqual�[2m(�[22m�[32mexpected�[39m�[2m) // deep equality�[22m

Expected: �[32mStringMatching /success|activeError/g�[39m
Received: �[31m"failed"�[39m
🟡 15 flaky test(s) (passed on retry)
  • Features/TeamsDragAndDrop.spec.ts › Should drag and drop on BusinessUnit team type (shard 1, 1 retry)
  • Pages/Customproperties-part1.spec.ts › no duplicate card after update (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/CustomMetric.spec.ts › Table custom metric (shard 2, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Features/Table.spec.ts › Tags term should be consistent for search (shard 3, 1 retry)
  • Flow/PersonaFlow.spec.ts › Persona rename flow should work properly (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Api Collection (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Announcement create, edit & delete (shard 4, 1 retry)
  • Pages/Glossary.spec.ts › Add and Remove Assets (shard 5, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab IS visible for supported type: searchIndex (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)
  • Pages/Users.spec.ts › Check permissions for Data Steward (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@harshach harshach closed this Apr 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants