[GitHub] [airflow] r-richmond commented on a change in pull request #5206: [AIRFLOW-4434] fix support for impala in hive hook

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

r-richmond commented on a change in pull request #5206: [AIRFLOW-4434] fix support for impala in hive hook
URL: https://github.com/apache/airflow/pull/5206#discussion_r279794183
 
 

 ##########
 File path: airflow/hooks/hive_hooks.py
 ##########
 @@ -797,11 +801,14 @@ def _get_results(self, hql, schema='default', fetch_size=None, hive_conf=None):
                 contextlib.closing(conn.cursor()) as cur:
             cur.arraysize = fetch_size or 1000
 
-            env_context = get_context_from_env_var()
-            if hive_conf:
-                env_context.update(hive_conf)
-            for k, v in env_context.items():
-                cur.execute("set {}={}".format(k, v))
+            # not all query services (e.g. impala AIRFLOW-4434) support the set command
+            db = self.get_connection(self.hiveserver2_conn_id)
+            if not db.extra_dejson.get('run_set_variable_statements', 'true').lower() == 'false':
 
 Review comment:
   I had it as a string as I haven't yet seen other extra properties that were booleans and wanted to keep the formatting/syntax of the extra property consistent.
   
   i.e. 1 rather than 2
   
   1)  `'{"authMech": "kerberos", "issue_set_statements": "false"}'`
   2) `'{"authMech": "kerberos", "issue_set_statements": false}'` note: false has to be lower case  despite `False` being python's format or json.loads throws `raise JSONDecodeError("Expecting value", s, err.value)`
   
   
   As for the double negative I agree it is a little harder to parse but I thought it did a better job of keeping the behavior as is.
   
   For example, with the double negative a value of `'sure'` will still allow the set statements to be run whereas in your proposed example a value of `'sure'` will skip the set statements.
   Although, I suppose just deleting the parameter is what most people will do anyways.
   
   Anyways, I don't have a strong opinion either way so I'll change it if you confirm recommendation. I'm just curious what your opinion of that line of thought is.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services