[GitHub] [airflow] dstandish commented on pull request #28721: Use connection URI in SqliteHook

[GitBox <gi...@apache.org>]

dstandish commented on PR #28721:
URL: https://github.com/apache/airflow/pull/28721#issuecomment-1371378599

   > I have found a solution [f3eaec7](https://github.com/apache/airflow/commit/f3eaec7875fc7bc373069320cf72d5852dbbc812) which does not require the conn_type to be added on user side. PTAL @dstandish @eladkal 🙏
   
   nice, good thinking
   
   Ok, so, it looks right to escape for compatibility with Connection.  But it does appear it will change if hostname happens to have url eschape codes in it.  but, to me this is like not a normal thing to do so, probably fine to ignore.   
   
   however, i did look and Connection.from_uri does not escape the `host`... so we would be inconsistent with its parsing of uri in that respect. again probably maybe ok?  
   
   But i did look to see what postgres does, and it doesn't escape.  Maybe sqlalchemy handles the escaping?  Can you confirm that before we merge?  If it's good enough for postgres,  maybe we don't need it here? It is probably worth adding a single test for URI behavior and a single one verifying that `sqlite3.connect` is called with the right value.


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org