potiuk commented on code in PR #66751:
URL: https://github.com/apache/airflow/pull/66751#discussion_r3680688526
##########
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:
Splitting on every `.` breaks table names that are already quoted and
contain a dot. Running the helper as written:
```python
>>> ".".join(_quote_presto_identifier(p) for p in '"my.table"'.split("."))
'"""my"."table"""'
>>> ".".join(_quote_presto_identifier(p) for p in 'db."odd.name"'.split("."))
'db."""odd"."name"""'
```
The dot inside the quoted identifier is treated as a catalog/schema
separator, each fragment fails both regexes, and both halves get re-escaped
into nonsense. Plain `db.tbl` and bare `tbl` are handled correctly, so this
only bites the quoted-with-dot case — but that's precisely the case the quoting
support exists for.
Splitting only on dots *outside* quotes would fix it, e.g.
`re.findall(r'"(?:[^"]|"")*"|[^.]+', self.table)`.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
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:
Minor, and I'd take your judgement over mine here: the pass-through for
already-quoted identifiers means the function's output depends on how the
caller happened to format the input, so `weird-col` and `"weird-col"` take
different paths to the same result. The comment above explains the intent well,
so this is fine as-is — but a couple of unit cases pinning the pass-through
behaviour (bare, pre-quoted, embedded-quote, and the dotted case above) would
stop a future refactor quietly re-escaping things.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
