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

Use DataQuery in Compiler instead of render DECLARE in DBAPI #21

Merged
merged 11 commits into from
Dec 15, 2023

Conversation

LuckySting
Copy link
Contributor

@LuckySting LuckySting commented Dec 8, 2023

Problem

Now rendering of a DECLARE clause happens in the Cursor.execute, which is bad because of the following.

  1. At the execution stage the information about the bound variable is incomplete, it impossible to use powerful SQLAlchemy type engine, so we able only guess about the bound variable type by the value.
  2. It becomes harder to change the Cursor implementation, because you have to implement the DECLARE rendering again.
  3. It is impossible to use SQLAlchemy caching of rendered queries, because the query becomes completely rendered right before the execution.

Suggested solution

Move rendering of a DECLARE clause to the Compilation stage, use information about the table and SQLAlchemy Type engine to determine types.

Workaround

It's impossible to render the DECLARE clause at the Compilation stage, because of "postcompile" variables. Instead it is possible to determine types of the bound variables right before the execution and pass it to the DataQuery.

Results

All tests are passed, even those that were previously failed.


def render_declare(self, post_compile_parameters: Union[list[dict], dict]) -> str:
if isinstance(post_compile_parameters, list):
common_keys = set.intersection(*map(set, post_compile_parameters))
Copy link
Member

Choose a reason for hiding this comment

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

It is difficult to read and understand many operations in one line.
Better to split code for simpler steps - for easy read in future.

parameter_names_sets = [set(parameters_dict.keys()) for parameters_dict in post_compile_parameters]
common_keys = set.intersection(parameter_names_sets)

post_compile_parameters_new = {}
for k in common_keys:
    values = [dic[k] for dic in post_compile_parameters]
    post_compile_parameters_new[k] = values
post_compile_parameters = post_compile_parameters_new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to make code easier to read

ydb_sqlalchemy/sqlalchemy/__init__.py Outdated Show resolved Hide resolved
if stype is None:
raise ProgrammingError(f"Cannot translate value {value} (type {tvalue}) to ydb type.")
class YdbOperation:
__slots__ = ("yql_text", "is_ddl", "parameters_types")
Copy link
Member

Choose a reason for hiding this comment

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

What about:
use dataclasses with default values?

Similar to ydb sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

from sqlalchemy.util.compat import inspect_getfullargspec

from typing import Any
from typing import Any, Union, Mapping, Sequence, Optional, Tuple
Copy link
Member

Choose a reason for hiding this comment

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

It will be great to explicit declare all public interface in all (similar to ydb sdk, topics), may be later - before release.

And hide all modules as implementation details to private (_prefix) files if them already imported to root of namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that we should do it before the release

@LuckySting LuckySting changed the title Move rendering of a DECLARE clause to the Compiler Use DataQuery in Compiler instead of render DECLARE in DBAPI Dec 12, 2023

return sql.replace("%%", "%"), sql_params
@dataclasses.dataclass
class YdbOperation:
Copy link
Member

Choose a reason for hiding this comment

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

ydb has own object "Operation" and YdbOperation may confuse to understand code.

What about rename the class to YdbQuery (because the class always use for queries)?

And move is_ddl above parameters_types with default value False, for user can omit the parameter while make most queries (DML query with params)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, done

@rekby rekby merged commit d413ef3 into ydb-platform:main Dec 15, 2023
5 checks passed
@LuckySting LuckySting deleted the declare-clause-compilation branch January 8, 2024 06:25
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.

2 participants