Skip to content

Conversation

@catsona
Copy link
Contributor

@catsona catsona commented Sep 22, 2025

No description provided.

@catsona catsona force-pushed the catsona/bi-6586/support_ydb_view branch from a575e6c to 8dde00c Compare September 23, 2025 12:33
@catsona catsona force-pushed the catsona/upgrade_ydb_sdk branch 3 times, most recently from 02ef2a5 to 9f1aeeb Compare September 26, 2025 08:24
@catsona catsona force-pushed the catsona/bi-6586/support_ydb_view branch 2 times, most recently from 08f6a01 to e80cc55 Compare September 26, 2025 10:06
@catsona catsona force-pushed the catsona/upgrade_ydb_sdk branch from 234fada to a0f6652 Compare October 10, 2025 09:39
@catsona catsona force-pushed the catsona/upgrade_ydb_sdk branch 3 times, most recently from 4156d76 to 01e92a0 Compare October 28, 2025 13:25
@catsona catsona force-pushed the catsona/upgrade_ydb_sdk branch from 636eec7 to 6d54f1d Compare October 29, 2025 10:14
@catsona catsona force-pushed the catsona/upgrade_ydb_sdk branch 2 times, most recently from 574a010 to 9575ee3 Compare November 25, 2025 15:30
@catsona catsona force-pushed the catsona/upgrade_ydb_sdk branch from 9575ee3 to 333c80c Compare November 26, 2025 11:42
Base automatically changed from catsona/upgrade_ydb_sdk to main November 26, 2025 14:01
@catsona catsona force-pushed the catsona/bi-6586/support_ydb_view branch from e80cc55 to 304098d Compare November 27, 2025 12:20
@catsona catsona marked this pull request as ready for review November 27, 2025 14:51
@ovsds ovsds requested a review from Copilot December 10, 2025 08:43
Copy link
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 adds support for YDB VIEW objects by enabling views to be treated similarly to tables in queries and dataset operations. The changes enable YDB views functionality through feature flags and update the connector to properly handle view objects alongside tables.

  • Enables YDB view support via feature flag configuration
  • Updates table listing logic to include view objects
  • Implements custom column info retrieval for views using subselect queries

Reviewed changes

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

Show a summary per file
File Description
lib/dl_sqlalchemy_ydb/dl_sqlalchemy_ydb/dialect.py Updates type hint to accept integer values in interval processing
lib/dl_connector_ydb/pyproject.toml Fixes relative path reference for dl-sqlalchemy-ydb dependency
lib/dl_connector_ydb/docker-compose.yml Adds feature flag to enable views in YDB test environment
lib/dl_connector_ydb/dl_connector_ydb_tests/db/api/test_dataset.py Adds test class for view-based datasets
lib/dl_connector_ydb/dl_connector_ydb_tests/db/api/base.py Implements test fixture base class for YDB view testing
lib/dl_connector_ydb/dl_connector_ydb/core/ydb/adapter.py Updates table listing to include view objects
lib/dl_connector_ydb/dl_connector_ydb/core/base/adapter.py Implements custom column info retrieval for views using subselects

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

table_path = table_def.table_name
else:
# Not ok?
raise ValueError("absolute table path is not subpath of database path")
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The error message should be more descriptive and include the actual paths to help debugging. Consider: f'Absolute table path {table_def.table_name} is not a subpath of database path {table_def.db_name}'

Suggested change
raise ValueError("absolute table path is not subpath of database path")
raise ValueError(
f"Absolute table path '{table_def.table_name}' is not a subpath of database path '{table_def.db_name}'"
)

Copilot uses AI. Check for mistakes.
Comment on lines +147 to +149
else:
# Not ok?
raise ValueError("absolute table path is not subpath of database path")
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Is this scenario actually possible? If it is, let's create a test for it
  2. Is ValueError suitable here? Won't it fire as a 5xx from the API?

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.

3 participants