-
Notifications
You must be signed in to change notification settings - Fork 113
Support SQLAlchemy ARRAY partial UPDATE and slice resizing #776
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
Changes from all commits
054200d
a322fa1
bc37103
b91e309
13a907d
23f7b19
97a5b6b
6280ee8
e58f4e0
8f3f237
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,9 +8,10 @@ | |
| from decimal import Decimal | ||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from sqlalchemy import cast, exc, types | ||
| from sqlalchemy import cast, exc, types, util | ||
| from sqlalchemy.sql import operators, sqltypes | ||
| from sqlalchemy.sql.elements import ColumnElement, Slice | ||
| from sqlalchemy.sql.elements import BinaryExpression, BindParameter, ColumnElement, Null, Slice | ||
| from sqlalchemy.sql.schema import Column | ||
| from sqlalchemy.sql.type_api import TypeEngine | ||
| from sqlalchemy.sql.visitors import InternalTraversal | ||
|
|
||
|
|
@@ -220,23 +221,23 @@ def has_unknown_element(type_: TypeEngine[Any]) -> bool: | |
|
|
||
|
|
||
| class _ArrayValueProcessor: | ||
| """Convert one declared ARRAY type between Python values and Athena transport. | ||
| """Convert ARRAY values and their typed elements to and from Athena transport. | ||
|
|
||
| SQLAlchemy constructs processors per type and dialect. Keep that context | ||
| here and share the recursive ARRAY/MAP/ROW traversal across bind parameters, | ||
| SQL literals, and fetched JSON results. | ||
| """ | ||
|
|
||
| def __init__(self, array_type: AthenaArray, dialect: Any) -> None: | ||
| self.array_type = array_type | ||
| def __init__(self, type_: TypeEngine[Any], dialect: Any) -> None: | ||
| self.type_ = type_ | ||
| self.dialect = dialect | ||
| self._type_inspector = _ArrayTypeInspector(dialect) | ||
|
|
||
| def bind(self, value: Any) -> Any: | ||
| return self._bind(value, self.array_type) | ||
| return self._bind(value, self.type_) | ||
|
|
||
| def literal(self, value: Any) -> str: | ||
| return self._literal(value, self.array_type) | ||
| return self._literal(value, self.type_) | ||
|
|
||
| def result(self, value: Any) -> Any: | ||
| if value is None: | ||
|
|
@@ -250,7 +251,11 @@ def result(self, value: Any) -> Any: | |
| return value | ||
| if isinstance(value, dict) and "_pyathena_array" in value: | ||
| value = value["_pyathena_array"] | ||
| return self._decode(value, self.array_type, self.array_type.as_tuple) | ||
| return self._decode( | ||
| value, | ||
| self.type_, | ||
| self.type_.as_tuple if isinstance(self.type_, sqltypes.ARRAY) else False, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _complex_values(value: Any, type_: TypeEngine[Any]): | ||
|
|
@@ -399,3 +404,246 @@ def _decode(self, value: Any, type_: TypeEngine[Any], as_tuple: bool = False) -> | |
| processor = type_.dialect_impl(self.dialect).result_processor(self.dialect, None) | ||
| return processor(value) if processor else value | ||
| return value | ||
|
|
||
|
|
||
| class _ArrayAssignmentType(types.TypeDecorator[Any]): | ||
| """Preserve declared element processors for ARRAY assignment values.""" | ||
|
|
||
| impl = types.NullType | ||
| cache_ok = True | ||
|
|
||
| def __init__(self, item_type): | ||
| super().__init__() | ||
| self.item_type = item_type | ||
|
|
||
| def bind_processor(self, dialect): | ||
| processor = _ArrayValueProcessor(self.item_type, dialect) | ||
|
|
||
| def process(value): | ||
| value = processor.bind(value) | ||
| if isinstance(value, (bytes, bytearray)): | ||
| return _ComplexParameter("FROM_HEX", (value.hex(),)) | ||
| return value | ||
|
|
||
| return process | ||
|
|
||
| def literal_processor(self, dialect): | ||
| return _ArrayValueProcessor(self.item_type, dialect).literal | ||
|
|
||
| def bind_expression(self, bindvalue): | ||
| expression = self.item_type.bind_expression(bindvalue) | ||
| return bindvalue if expression is None else expression | ||
|
|
||
|
|
||
| class _ArrayWriteIndexType(types.TypeDecorator[int]): | ||
| """Reject non-integer and NULL bound ARRAY write indices.""" | ||
|
|
||
| impl = types.Integer | ||
| cache_ok = True | ||
|
|
||
| def process_bind_param(self, value, dialect): | ||
| if type(value) is not int: | ||
| raise ValueError("ARRAY write indices must be non-NULL integers") | ||
| return value | ||
|
|
||
| def process_literal_param(self, value, dialect): | ||
| return self.process_bind_param(value, dialect) | ||
|
|
||
|
|
||
| class _ArrayUpdate(ColumnElement[Any]): | ||
| """Whole-column expression generated from one partial ARRAY assignment.""" | ||
|
|
||
| __visit_name__ = "athena_array_update" | ||
| inherit_cache = True | ||
| _traverse_internals = [ # noqa: RUF012 | ||
| ("column", InternalTraversal.dp_clauseelement), | ||
| ("path", InternalTraversal.dp_clauseelement_list), | ||
| ("value", InternalTraversal.dp_clauseelement), | ||
| ("type", InternalTraversal.dp_type), | ||
| ] | ||
|
|
||
| def __init__(self, column, path, value, value_type): | ||
| self.column = column | ||
| self.path = path | ||
| self.type = column.type | ||
| self.value_type = value_type | ||
| self.value = ( | ||
| value._with_binary_element_type( | ||
| _ArrayAssignmentType(value_type if value.type._isnull else value.type) | ||
| ) | ||
| if isinstance(value, BindParameter) | ||
| else value | ||
| ) | ||
|
|
||
| @property | ||
| def _from_objects(self): | ||
| return self.column._from_objects + self.value._from_objects | ||
|
|
||
| @classmethod | ||
| def rewrite(cls, statement, dialect): | ||
| inspector = _ArrayTypeInspector(dialect) | ||
| values = statement._ordered_values | ||
| if values is None: | ||
| values = list((statement._values or {}).items()) | ||
| rewritten = [] | ||
| seen = set() | ||
| partial = set() | ||
| for key, value in values: | ||
| base = key | ||
| path: list[Any] = [] | ||
| while isinstance(base, BinaryExpression) and base.operator is operators.getitem: | ||
| if inspector.array_type(base.left.type) is None: | ||
| break | ||
| path.insert(0, base.right) | ||
| base = base.left | ||
| name = base if isinstance(base, str) else getattr(base, "key", None) | ||
| if path: | ||
| if ( | ||
| not isinstance(base, Column) | ||
| or base.table is None | ||
| or base.table._deannotate() is not statement.table._deannotate() | ||
| ): | ||
| raise exc.CompileError("ARRAY updates require a column of the target table") | ||
| if name in seen: | ||
| raise exc.CompileError("Only one assignment per ARRAY column is supported") | ||
| if any(isinstance(index, Slice) for index in path[:-1]): | ||
| raise exc.CompileError("Only the final ARRAY update index can be a slice") | ||
| partial.add(name) | ||
| value_type = key.type | ||
| value = cls(base, path, value, value_type) | ||
| key = base | ||
| elif name in partial: | ||
| raise exc.CompileError("Only one assignment per ARRAY column is supported") | ||
| seen.add(name) | ||
| rewritten.append((key, value)) | ||
| if not partial: | ||
| return statement | ||
| result = statement._clone() | ||
| if statement._ordered_values is not None: | ||
| result._ordered_values = rewritten | ||
| else: | ||
| result._values = util.immutabledict(rewritten) | ||
| return result | ||
|
|
||
|
|
||
| class _ArrayUpdateCompiler: | ||
| """Render an ARRAY assignment by rebuilding its affected nested arrays.""" | ||
|
|
||
| def __init__(self, compiler): | ||
| self.compiler = compiler | ||
| self._type_inspector = _ArrayTypeInspector(compiler.dialect) | ||
|
|
||
| def process(self, expression, **kw): | ||
| compiler = self.compiler | ||
| value = expression.value | ||
| final_slice = isinstance(expression.path[-1], Slice) | ||
| if final_slice and ( | ||
| isinstance(value, Null) | ||
| or ( | ||
| isinstance(value, BindParameter) | ||
| and not value.required | ||
| and value.callable is None | ||
| and value.value is None | ||
| ) | ||
| ): | ||
| raise exc.CompileError("An ARRAY slice assignment requires a non-NULL array") | ||
| rhs = compiler.process(value, **kw) | ||
| rhs_type = compiler._complex_dml_type(expression.value_type, require_precision=True) | ||
| rhs = f"CAST({rhs} AS {rhs_type})" | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Independent review (relayed): FINDINGS. Claude claude-fable-5-1 / verified first-party Max / effort high; session dd66cd12-7dbd-4fb8-9c53-2b72b047522c. Full static review of d3b296c..4dac6f5 covered all six changed files, direct compiler/formatter/type/ORM/cache paths and dependency sources. Read/Glob/Grep only; no execution, edits, network, PR discussion or memory access. Actual result collected; snapshot and worktree unchanged. The reviewer flagged unconditional RHS casts as a possible precision loss and noted that Numeric() SQL-expression assignments bypass the bound-value guard. The generated bare DECIMAL cast is confirmed by compilation; the repair will require declared decimal precision for SQL-expression assignments as well as bound values. The proposed wholesale removal of target-type casts is not applied on the unverified timestamp premise; supported timestamp precision will be checked in a real update. The timestamp(6) premise is not established for Athena: AWS explicitly documents millisecond precision for both Iceberg reads and writes (https://docs.aws.amazon.com/athena/latest/ug/querying-iceberg.html). We will not claim microsecond preservation from Trino assumptions. The reviewer also questioned whether the 10,001-element setup INSERT can run. That scenario already passed in both synchronous and asynchronous real-Athena tests, so it is not a confirmed failure. No general sequence-size or unlimited-array claim is made.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disposition at 6f2bebe: the SQL-expression decimal guard gap is repaired; both element and slice expression cases now require declared precision. Target-type casts are retained rather than removed on the unverified timestamp premise. Real sync/async UPDATE tests preserve millisecond timestamps and decimal expression values within Athena's supported contract. No microsecond-preservation claim is made. The long-array INSERT and UPDATE already passed in the initial sync/async suite; no setup change was needed. Both self-review perspectives were reapplied; independent follow-up is pending.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Independent narrow follow-up (relayed): FINDINGS, all low severity, no blocking defect; the reviewer confirmed the production repair is correct. Claude claude-fable-5-1 / Max / high, session 451eaea0-a1a9-470c-9154-e0551c594988, statically reviewed only 4dac6f5..6f2bebe, its range comparison, focused public excerpts, and the AWS contract excerpt (four files, about 32 KB). No other files were accessed; package and worktree were unchanged during review. The reviewer confirmed the decimal guard, millisecond fixtures, and literal-cache tests, which would fail on a cache miss. Remaining items: (1) moving processor assertions lost verification that compilation attaches the index validator; (2) the expression decimal case replaced the prior bound-element runtime case; (3) callable compiler checks and a leftover 2-D test setup should be placed more precisely, with new classes ordered by responsibility; (4) precision-only validation and its helper/error wording should not imply enforced scale or binds-only use. Restoring both coverage paths and clarifying the internal keyword, error and documentation; both author review perspectives and a further bounded follow-up will be collected. The reviewer did not execute the Timestamp case; author runtime verification remains separately recorded.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relayed independent follow-up: CLEAN. Claude Covered the precision-keyword rename and callers, revised exception/precision documentation, test class responsibilities, compiler processor wiring, and the restored decimal round-trip. No blocking findings. The reviewer noted an optional split of the callable index/slice compile test at Package limits were checked locally: no
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final validation for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Independent post-merge rebase follow-up (Claude Code
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Self-review round 1 of the ARRAY compiler simplification: CLEAN for repair delta
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Self-review round 2 of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Independent refactor follow-up (Claude Code
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final readiness verification for |
||
| if final_slice: | ||
| # Reject SQL expressions that evaluate to NULL without issuing a second statement. | ||
| failure = ( | ||
| f"slice(CAST(ARRAY[] AS {rhs_type}), " | ||
| "CAST(concat('NULL ARRAY slice assignment', coalesce(CAST(cardinality(" | ||
| f"{rhs}) AS VARCHAR), '')) AS BIGINT), 0)" | ||
| ) | ||
| rhs = f"IF({rhs} IS NULL, {failure}, {rhs})" | ||
| return self._rebuild( | ||
| compiler.process(expression.column, **kw), expression.type, expression.path, rhs, **kw | ||
| ) | ||
|
|
||
| def _index_sql(self, index: ColumnElement[Any], **kw): | ||
| compiler = self.compiler | ||
| if isinstance(index, Null): | ||
| raise exc.CompileError("ARRAY write indices must be non-NULL positive integers") | ||
| if ( | ||
| isinstance(index, BindParameter) | ||
| and not index.required | ||
| and index.callable is None | ||
| and (type(index.value) is not int or index.value <= 0) | ||
| ): | ||
| raise exc.CompileError( | ||
| "ARRAY write indices must be positive integers after normalization" | ||
| ) | ||
| if not isinstance(index.type, (types.Integer, types.NullType)) and not ( | ||
| isinstance(index, BindParameter) | ||
| and self._type_inspector.array_type(index.type) is not None | ||
| ): | ||
| raise exc.CompileError("ARRAY write indices must be integers") | ||
|
|
||
| if isinstance(index, BindParameter): | ||
| index = index._with_binary_element_type(_ArrayWriteIndexType()) | ||
| sql = compiler.process(index, **kw) | ||
| failure = ( | ||
| "CAST(concat('Invalid ARRAY index: ', " | ||
| f"coalesce(CAST({sql} AS VARCHAR), 'NULL')) AS BIGINT)" | ||
| ) | ||
| return f"IF({sql} > 0, {sql}, {failure})" | ||
|
|
||
| def _rebuild(self, array, array_type, path, rhs, **kw): | ||
| compiler = self.compiler | ||
| array_type = self._type_inspector.array_type(array_type) | ||
| if array_type is None: | ||
| raise exc.CompileError("Partial ARRAY updates require an ARRAY column type") | ||
| array_sql_type = compiler._complex_dml_type(array_type) | ||
| array = f"coalesce({array}, CAST(ARRAY[] AS {array_sql_type}))" | ||
| bound = path[0] | ||
| if isinstance(bound, Slice): | ||
| return self._rebuild_slice(array, array_type, bound, rhs, **kw) | ||
| return self._rebuild_element(array, array_type, bound, path[1:], rhs, **kw) | ||
|
|
||
| def _prefix_and_padding(self, array, start, array_type): | ||
| prefix = f"slice({array}, 1, least({start} - 1, cardinality({array})))" | ||
| element_type = self.compiler._complex_dml_type(_ArrayTypeInspector.item_type(array_type)) | ||
| padding = ( | ||
| f"repeat(CAST(NULL AS {element_type}), " | ||
| f"CAST(greatest({start} - 1 - cardinality({array}), 0) AS INTEGER))" | ||
| ) | ||
| return prefix, padding | ||
|
|
||
| def _rebuild_slice(self, array, array_type, bound, rhs, **kw): | ||
| if not isinstance(bound.step, Null) and not ( | ||
| isinstance(bound.step, BindParameter) | ||
| and bound.step.unique | ||
| and type(bound.step.value) is int | ||
| and bound.step.value == 1 | ||
| ): | ||
| raise exc.CompileError("Athena ARRAY slices support only step=None or step=1") | ||
| start = "1" if isinstance(bound.start, Null) else self._index_sql(bound.start, **kw) | ||
| stop = ( | ||
| f"cardinality({array})" | ||
| if isinstance(bound.stop, Null) | ||
| else self._index_sql(bound.stop, **kw) | ||
| ) | ||
| prefix, padding = self._prefix_and_padding(array, start, array_type) | ||
| tail_start = f"greatest({start}, {stop} + 1)" | ||
| suffix = ( | ||
| f"slice({array}, {tail_start}, greatest(cardinality({array}) - {tail_start} + 1, 0))" | ||
| ) | ||
| return self.compiler._array_slice_step( | ||
| f"concat({prefix}, {padding}, {rhs}, {suffix})", bound.step, array_type, **kw | ||
| ) | ||
|
|
||
| def _rebuild_element(self, array, array_type, bound, remaining_path, rhs, **kw): | ||
| index = self._index_sql(bound, **kw) | ||
| previous = f"element_at({array}, {index})" | ||
| replacement = ( | ||
| self._rebuild( | ||
| previous, _ArrayTypeInspector.item_type(array_type), remaining_path, rhs, **kw | ||
| ) | ||
| if remaining_path | ||
| else rhs | ||
| ) | ||
| prefix, padding = self._prefix_and_padding(array, index, array_type) | ||
| suffix = f"slice({array}, {index} + 1, greatest(cardinality({array}) - {index}, 0))" | ||
| return f"concat({prefix}, {padding}, ARRAY[{replacement}], {suffix})" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review round two — claims, caller compatibility, and AWS operations: CLEAN. Full initial inventory at d3b296c..4dac6f5 includes all six changed files, PR description, new class docstrings, and partial-update documentation.
Traced the one-statement claim through visit_update and the generated whole-column expression: the rewrite is local compilation and adds no metadata fetch, client-side read/modify/write, retry, or account configuration. Existing WHERE/ordered assignments and target table identity remain intact. Checked every documented resize boundary against the integration case matrix, including NULL destinations, empty deletes, reversed insertion, out-of-range padding and invalid replacement rejection. The long-array/scalar-subquery claim is limited to the measured 10,001-element and scalar-subquery cases; repeat limits and deterministic expression requirements remain explicit. The example now identifies an existing Iceberg table and reflects its ARRAY column rather than using an undefined table variable.
Checked typed and callable bindings, decorators, binary/decimal transport and the parent precision rule, plus SQLAlchemy 2.0.0 compile compatibility. Test classes/fixtures retain the existing conventions and all 156 inherited method ASTs are preserved. Validation claims distinguish six synchronous cases before the decimal guard plus the affected repair case, current six asynchronous cases, 243 pure cases, and the current-source HTML build (154 warnings). The standard multiversion build targets master/tags and is not substituted for the current-source build. Full current CI and independent review remain pending. No additional actionable claim or operational issue found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review round two repair: CLEAN. Repair scope: 4dac6f5..6f2bebe, with the same merge base d3b296c and a patch-series comparison. The docs now require declared decimal precision for expression assignments and explain validation-phase/exception differences with cached statements. Target-type casts remain deliberate: the proposed microsecond-loss premise was not applicable to Athena's documented millisecond read/write support (https://docs.aws.amazon.com/athena/latest/ug/querying-iceberg.html), and synchronous/asynchronous tests preserve 456 ms Timestamp values and 4.56 decimal expression results. The new cache test observes DBAPIError from both invalid-index and NULL-slice guards and verifies unchanged data. Earlier long-array setup already passed on real Athena, so the unverified bigint INSERT concern is not treated as a defect. The current-source doc rebuild passes (56 incremental Sphinx warnings). Current aggregate CI and independent follow-up remain pending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repair scope:
6f2bebebca87aefc05ccf6bfa798da263169269b..9d260111e10600355c8b9b1b23e4b7e5f67698bf; frozen PR merge based3b296cdfa63dff4b5cd06b4949324a810b19684. The initial full review remains recorded above. Round two follow-up: CLEAN. Audited the revised precision and exception claims: Numeric precision is required, scale is needed to preserve fractional values; compilation/binding/server validation may expose CompileError/StatementError/DBAPIError. The private keyword has no remaining old callers. SQLAlchemy 2.0.0 compilation smoke, Markdown lint and current-source HTML build pass. Bound decimal 1.23 and expression result 4.56 were both verified through sync and async connections. Full current-head CI is pending; these targeted runs are not represented as a completed full matrix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase follow-up:
d3b296cdfa63dff4b5cd06b4949324a810b19684..9d260111e10600355c8b9b1b23e4b7e5f67698bfcompared withfbcf8b88d845eb7577d5c481fad0fb265e832ebb..a9c8c18330413bf47e1bb0d9fa47a84d13bb3c06. Initial full reviews and repair follow-ups remain recorded above. Round two: CLEAN. Checked the revised PR body against source and evidence: the full current source/tests are identical to the previously reviewed head, while old real-Athena and CI claims are labeled prior-revision evidence. Closes #755 is present; automatic closure is conditional on eventual merge to the default branch. The new contribution template is followed with WHAT/WHY/TEST and a concrete validation record; all six changed files already exist, so no new-file header applies. Updated CI admission permits the same-repository branch. Current-head CI is pending, and 154 Sphinx warnings belong to the successful local build.