Har1sh-k commented on code in PR #66751:
URL: https://github.com/apache/airflow/pull/66751#discussion_r3809238734
##########
providers/apache/hive/src/airflow/providers/apache/hive/operators/hive_stats.py:
##########
@@ -126,15 +157,21 @@ def execute(self, context: Context) -> None:
assign_exprs = self.get_default_exprs(col, col_type)
exprs.update(assign_exprs)
exprs.update(self.extra_exprs)
- exprs_str = ",\n ".join(f"{v} AS {k[0]}__{k[1]}" for k, v in
exprs.items())
+ exprs_str = ",\n ".join(
+ f"{v} AS {_quote_presto_identifier(f'{k[0]}__{k[1]}')}" for k, v
in exprs.items()
+ )
- where_clause_ = [f"{k} = '{v}'" for k, v in self.partition.items()]
+ presto = PrestoHook(presto_conn_id=self.presto_conn_id)
+ # Build the WHERE clause against the hook's declared parameter
placeholder
+ # (PrestoHook defaults to `?`; a connection may override it via the
`placeholder` extra).
+ placeholder = presto.placeholder
+ where_clause_ = [f"{_quote_presto_identifier(k)} = {placeholder}" for
k in self.partition.keys()]
where_clause = " AND\n ".join(where_clause_)
- sql = f"SELECT {exprs_str} FROM {self.table} WHERE {where_clause};"
+ quoted_table = ".".join(_quote_presto_identifier(part) for part in
self.table.split("."))
Review Comment:
Fixed in 62943d0. Moved the assembly into `_quote_presto_table`, using the
regex you suggested so that only the dots outside a quoted identifier separate
components:
```python
_QUALIFIED_NAME_PART_RE = re.compile(r'"(?:[^"]|"")*"|[^.]+')
def _quote_presto_table(table: str) -> str:
return ".".join(_quote_presto_identifier(part) for part in
_QUALIFIED_NAME_PART_RE.findall(table))
```
Both of your reproducers now come back correct: `"my.table"` stays
`"my.table"` and `db."odd.name"` stays `db."odd.name"`. Plain `db.tbl` and bare
`tbl` are byte-identical to before. Both are parametrized cases in
`test_quote_presto_table` and in `test_execute_quotes_table_identifier`.
One caveat I put in the PR comment rather than bury here: this case cannot
actually reach the `FROM` clause today, because `HiveMetastoreHook.get_table`
raw-splits the same string on line 703-704 before the operator builds any SQL.
##########
providers/apache/hive/src/airflow/providers/apache/hive/operators/hive_stats.py:
##########
@@ -29,6 +30,33 @@
if TYPE_CHECKING:
from airflow.providers.common.compat.sdk import Context
+# The table, the partition columns, and the metastore columns projected in the
Presto
+# stats SELECT are interpolated as identifiers, which cannot be bound as SQL
parameters.
+# Plain word identifiers are emitted unchanged, and identifiers the caller
already
+# double-quoted correctly are passed through as-is (so a pre-quoted name such
as
+# ``"weird-col"`` is not re-escaped into ``"""weird-col"""``); anything else is
+# double-quoted with embedded quotes doubled (how Presto/Trino escape
identifiers).
+# Kept local rather than reusing common.sql's ``Dialect.escape_word``, which
needs a
+# live connection and does not double embedded quotes.
+_PLAIN_IDENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*")
+# A fully and correctly double-quoted identifier: opening and closing quotes
with every
+# embedded quote doubled (e.g. ``"a""b"``). Used to detect identifiers the
caller has
+# already escaped so they are left untouched instead of being double-escaped.
+_QUOTED_IDENT_RE = re.compile(r'"(?:[^"]|"")*"')
+
+
+def _quote_presto_identifier(identifier: str) -> str:
Review Comment:
Added `test_quote_presto_table` pinning exactly those: bare,
qualified-plain, per-component special characters, pre-quoted, embedded doubled
quote, and the dotted case, plus a trailing-SQL input that stays one inert
component. The dotted case is also asserted at the emitted-SQL level in
`test_execute_quotes_table_identifier`, so the behaviour is pinned at both the
helper and the generated SQL.
To check the new cases are not vacuous, I reverted the helper body to the
old `split(".")` and confirmed 6 of the new assertions fail and only those.
Full module: 41 passed, 1 skipped (the pre-existing `AIRFLOW_RUNALL_TESTS`
skip).
I kept the pass-through as-is per your note. It does mean `weird-col` and
`"weird-col"` reach the same result by different branches, but making the
function canonicalize instead would mean unwrapping and re-escaping
caller-quoted names, which is the behaviour Nataneljpwd flagged earlier in the
review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
