Skip to content

Fix multi-row CAST when bind names contain escape-map characters (PECOBLR-2746)#69

Open
msrathore-db wants to merge 1 commit into
mainfrom
fix/pecoblr-2746-followup-escaped-bindnames
Open

Fix multi-row CAST when bind names contain escape-map characters (PECOBLR-2746)#69
msrathore-db wants to merge 1 commit into
mainfrom
fix/pecoblr-2746-followup-escaped-bindnames

Conversation

@msrathore-db

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #68 (PECOBLR-2746). The merged PR wraps mixed-type multi-row INSERT binds with CAST(... AS STRING) via post-compile string replacement, but the marker it builds uses the raw bind name from self.binds. SQLAlchemy renders the marker using the escaped form after applying bindname_escape_characters ( /./[/]/(/)/%/:_ etc.). When the column name contains any of those characters the two strings don't match, str.replace is a no-op, no CAST is emitted, and the mixed-type multi-row insert fails again with INVALID_INLINE_TABLE.INCOMPATIBLE_TYPES_IN_INLINE_TABLE.

Customer reproduced this on the merged fix with column names containing spaces and dots (see PECOBLR-2746 Comment 21).

Fix

Look up the escaped form via self.escaped_bind_names when rebuilding the marker:

rendered_name = self.escaped_bind_names.get(bind_name, bind_name)
marker = self._BIND_TEMPLATE % {"name": rendered_name.replace("`", "``")}

Backtick-containing names take a different path in bindparam_string that bypasses the escape map entirely; the existing .replace("\", "``")` doubling matches that path too, so pure backticks and backticks combined with escape-map characters are handled by the same code.

Tests

New TestMultiRowInsertCastsEscapedBindNames class in tests/test_local/test_ddl.py:

  • Parametrized over every char in bindname_escape_characters — space, ., [, ], (, ), %, :.
  • Literal backtick column name.
  • Backtick + dot in the same column name.
  • Mixed escape-char columns in the same table.

All compile-only — no warehouse needed.

Test plan

  • tests/test_local/test_ddl.py — 39 passed (10 new)
  • tests/test_local/ (all non-e2e) — 290 passed
  • E2E against pecotesting — 9/9 scenarios pass: clean, col with space, col.dot, col[bracket], col(paren), col%pct, col:colon, col`tick, col`x.y. Each runs df.to_sql(..., method='multi') with mixed int/int/str rows in the named column.

Out of scope

Audit found two pre-existing identifier-handling issues unrelated to PR #68 — three raw-f-string sites in base.py for DESCRIBE/SHOW reflection that don't double backticks, and a charset-restricted regex plus a naive strip_backticks in _parse.py that mis-extract FK column names. Both are separate bug classes from the inline-table issue and will be filed as their own follow-ups; this PR stays scoped to the multi-row cast path.

This pull request and its description were written by Isaac.

PR #68 wraps mixed-type multi-row INSERT binds with CAST(... AS STRING) by
post-editing the rendered SQL. Marker reconstruction used the raw bind name
from ``self.binds``, but SQLAlchemy renders the marker after applying
``bindname_escape_characters`` (space/./[/]/(/)/%/: get translated). For any
column name containing one of those chars the two strings disagree,
``str.replace`` is a no-op, and the mixed-type insert fails again with
``INCOMPATIBLE_TYPES_IN_INLINE_TABLE`` (PECOBLR-2746 follow-up, Comment 21).

Look up the escaped form via ``self.escaped_bind_names`` so the rebuilt
marker matches what the compiler actually wrote. Backtick-containing names
take a different path in ``bindparam_string`` that bypasses the escape map;
the ``.replace("\`", "\`\`")`` doubling in marker rebuild matches that path
too, so backticks alone, and backticks combined with escape-map chars, are
handled by the same code.

Tests cover every char in ``bindname_escape_characters`` plus literal and
combined backtick cases, all at compile-only level (no warehouse).

Co-authored-by: Isaac
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.

1 participant