-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add phoenix query runner. #3153
Conversation
We would appreciate it if you could provide us with more info about this issue/pr! |
@arikfr May I ask you for a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please see comment on error handling.
redash/query_runner/phoenix.py
Outdated
except Exception as ex: | ||
json_data = None | ||
error = 'error.{}.{}'.format(query, ex) | ||
if not isinstance(error, basestring): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why error want be a basestring
?
redash/query_runner/phoenix.py
Outdated
json_data = None | ||
except Exception as ex: | ||
json_data = None | ||
error = 'error.{}.{}'.format(query, ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is shown to the user and should be (to some degree) user friendly. Not sure this qualifies :)
Maybe returning just the exception message is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling is sufficient. Users can identify errors with code and message. Please refer to https://python-phoenixdb.readthedocs.io/en/latest/api.html#phoenixdb.Error.
@arikfr Thank you for your review. |
Thanks! |
* Add phoenix query runner. * Improved error handling.
This is an apache phoenix query runner. The query runner presently does not support Kerberos authentication. But I plan to implement it soon.