eladkal commented on code in PR #38528:
URL: https://github.com/apache/airflow/pull/38528#discussion_r1551212901
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -50,6 +50,7 @@
from airflow.providers.openlineage.sqlparser import DatabaseInfo
T = TypeVar("T")
+DEFAULT_SQL_PLACEHOLDERS = frozenset({"%s", "?"})
Review Comment:
```suggestion
SQL_PLACEHOLDERS = frozenset({"%s", "?"})
```
It's not defaults it's allowed placeholders
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -173,7 +175,17 @@ def __init__(self, *args, schema: str | None = None,
log_sql: bool = True, **kwa
)
@property
- def placeholder(self) -> str:
+ def placeholder(self):
+ conn = self.get_connection(getattr(self, self.conn_name_attr))
+ placeholder = conn.extra_dejson.get("placeholder")
+ if placeholder in DEFAULT_SQL_PLACEHOLDERS:
+ return placeholder
+ self.log.warning(
+ "Placeholder defined in Connection '%s' is not listed in
'DEFAULT_SQL_PLACEHOLDERS' "
+ "and got ignored. Falling back to the default placeholder '%s'.",
+ placeholder,
Review Comment:
I wonder if this is right approach?
I would prefer to warn when using placeholder that is different with what we
know but if user still wish to use something that is different we should allow
it and not prevent it.
--
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]
