Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement disposition field in SQL backend #3477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JCZuurmond
Copy link
Member

Description

This feature aims to the chance of running SQL statement for assessment results export in case of large workspaces with a large amount of findings.
Introducing the query_statement_disposition value during the UCX installation can allow users to select the proper disposition method to use when running large queries. This parameter is added in the config.yml file and used for the SqlBackend definition. Using this approach, large queries will not fail.

Changes

Added SQL query statement disposition choice during the UCX installation
Added query_statement_disposition value in the config file

Linked issues

Resolves #3447
From other PR #3455

Functionality

  • modified existing commands: databricks labs install ucx, databricks labs ucx export-assessment

Tests

  • manually tested

@JCZuurmond JCZuurmond requested a review from a team as a code owner January 6, 2025 14:41
@JCZuurmond JCZuurmond changed the title Sql statement disposition feature Implement disposition field in SQL backend Jan 6, 2025
@JCZuurmond JCZuurmond added the pr/do-not-merge this pull request is not ready to merge label Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

✅ 24/24 passed, 1 flaky, 32m21s total

Flaky tests:

  • 🤪 test_running_real_remove_backup_groups_job (8m26.451s)

Running from acceptance #7919

Having this feature will allow the execution of queries with not negligible dimension results.
Using this field during the configuration, will allow to export assessment results for larger workspaces.
@JCZuurmond JCZuurmond force-pushed the sql-statement-disposition-feature branch from 9a7c8d1 to 780a3c6 Compare January 8, 2025 16:44
@JCZuurmond JCZuurmond removed the pr/do-not-merge this pull request is not ready to merge label Jan 8, 2025
@JCZuurmond JCZuurmond self-assigned this Jan 8, 2025
@@ -92,6 +93,9 @@ class WorkspaceConfig: # pylint: disable=too-many-instance-attributes
# Skip TACL migration during table migration
skip_tacl_migration: bool = False

# Select SQL query statement disposition, default to INLINE
query_statement_disposition: Disposition | None = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
query_statement_disposition: Disposition | None = None
query_statement_disposition: Disposition = Disposition.INLINE

@@ -259,6 +260,12 @@ def _prompt_for_new_installation(self) -> WorkspaceConfig:
recon_tolerance_percent = int(
self.prompts.question("Reconciliation threshold, in percentage", default="5", valid_number=True)
)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would reconsider. This may be confusing to the users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Add Disposition parameter in SqlBackend creation
3 participants