feat(connectors): BI-6611 DB_CAST for YDB without ARRAY support #1261
Conversation
catsona
changed the title
catsona
changed the title
catsona
changed the title
catsona
force-pushed
the
catsona/upgrade_ydb_sdk
branch
from
234fada to
a0f6652
Compare
catsona
force-pushed
the
catsona/upgrade_ydb_sdk
branch
3 times, most recently
from
4156d76 to
01e92a0
Compare
catsona
force-pushed
the
catsona/upgrade_ydb_sdk
branch
from
636eec7 to
6d54f1d
Compare
catsona
force-pushed
the
catsona/upgrade_ydb_sdk
branch
2 times, most recently
from
574a010 to
9575ee3
Compare
catsona
force-pushed
the
catsona/upgrade_ydb_sdk
branch
from
9575ee3 to
333c80c
Compare
catsona
force-pushed
the
catsona/bi-6611/ydb_db_cast
branch
from
e843d79 to
f7b7ddc
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements the DB_CAST function for YDB connector without ARRAY support, enabling YDB-specific type casting operations based on YDB's native type system and casting rules.
- Adds comprehensive DB_CAST implementation with YDB-specific type specifications and casting whitelists
- Implements extensive test coverage for various type casting combinations (bool, integer, float, string, date)
- Corrects the path to the dl-sqlalchemy-ydb dependency
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/dl_connector_ydb/pyproject.toml | Fixed dependency path for dl-sqlalchemy-ydb |
| lib/dl_connector_ydb/dl_connector_ydb_tests/db/formula/test_functions_type_conversion.py | Added comprehensive test suite for DB_CAST functionality with multiple type conversion scenarios |
| lib/dl_connector_ydb/dl_connector_ydb/formula/definitions/functions_type.py | Implemented DB_CAST function with YDB type specifications and casting whitelists based on YDB documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| source_column: str, | ||
| ) -> None: | ||
| if cast_args: | ||
| cast_args_str = ", ".join(cast_args) |
There was a problem hiding this comment.
The cast_args parameter is typed as tuple[int, int] | None, but str.join() expects an iterable of strings. This will fail at runtime when cast_args contains integers. Convert the integers to strings using cast_args_str = ', '.join(str(arg) for arg in cast_args) or cast_args_str = ', '.join(map(str, cast_args)).
| cast_args_str = ", ".join(cast_args) | |
| cast_args_str = ", ".join(str(arg) for arg in cast_args) |
| # TODO: JsonDocument | ||
| # TODO: Yson | ||
| # TODO: Cast Integer to Interval | ||
| # Commented - not supported |
There was a problem hiding this comment.
By the look at the table above I assume that this is not supported by YDB rather than us. Do we really need both the table and the commented lines? I would suggest removing those commented TYPES_SPECs and add a comment that all mappings are based on the table above
| @pytest.fixture(scope="function") | ||
| def ydb_data_test_table_field_types_patch(self, monkeypatch) -> None: | ||
| ydb_field_types = {**self.YDB_TYPE_FIELD_TYPES} | ||
|
|
||
| monkeypatch.setattr("dl_formula_testing.evaluator.FIELD_TYPES", ydb_field_types) | ||
|
|
||
| return ydb_field_types |
There was a problem hiding this comment.
I think this monkeypatch can be easily avoided by modifying the DbEvaluator in such way that it would accept type overrides. DbEvaluator.translate_formula() already accepts field_types, but it is not used at the moment. If I am wrong and it turns out that it's not an easy fix, then you may just leave a TODO here, we still would want to get rid of this monkeypatch eventually.
No description provided.