[Trino] Adding explain query method #3645
Conversation
agl29
requested review from
wing2fly,
amitsrivastava,
JohanAhlen,
athithyaaselvam and
Harshg999
agl29
force-pushed
the
trino_query_explain
branch
2 times, most recently
from
be2754e to
106a871
Compare
| result = query_client.execute() | ||
|
|
||
| return { | ||
| 'status': 0, |
There was a problem hiding this comment.
Will the TrinoQuery.execute() throws any exception?
If any exception, should the status be updated to -1?
Take a look existing beeswax API examples: response = {'status': -1, 'message': ''} are defined at the beginning of menthod.
There was a problem hiding this comment.
Added the try catch.
We need to send the {'status': 0} to capture the output in UI.
For the ref https://github.com/cloudera/hue/blob/master/desktop/libs/notebook/src/notebook/connectors/hiveserver2.py#L591
There was a problem hiding this comment.
@wing2fly we should try to move away from using the status flag, today API users expect the proper http error code when something goes bad. That being said the UI might have some old code that checks for status == 0.
agl29
force-pushed
the
trino_query_explain
branch
2 times, most recently
from
7b844f5 to
023df9d
Compare
agl29
force-pushed
the
trino_query_explain
branch
from
023df9d to
8d5bbc2
Compare
| result = query_client.execute() | ||
| explanation = result.rows | ||
| except Exception as e: | ||
| explanation = str(e) |
There was a problem hiding this comment.
Adding error in explanation?
There was a problem hiding this comment.
Yes, that's what we are doing in the Hive also
https://github.com/cloudera/hue/blob/master/desktop/libs/notebook/src/notebook/connectors/hiveserver2.py#L608
There was a problem hiding this comment.
@JohanAhlen Is UI expecting this for both editor v1 and editor v2?
Just a heads up, we need to improve all this flow then once we pick up this roadmap item. cc @agl29
There was a problem hiding this comment.
It's a bit odd indeed, @agl29 did you try how the UI behave if you would return a 500 request with the error message instead?
agl29
force-pushed
the
trino_query_explain
branch
2 times, most recently
from
3e340f6 to
ae00b20
Compare
agl29
force-pushed
the
trino_query_explain
branch
from
ae00b20 to
c4dcfa4
Compare
(cherry picked from commit 1f64dee) (cherry picked from commit cb74ef31234d72d22a282cac6a0b18fdda6891ce) Change-Id: I0d530fc3e68a8005cf2512b2733ef36401a4af74 (cherry picked from commit 38920cb119157f6c54794a18e4ad6949dcb8fac2)
(cherry picked from commit 1f64dee) (cherry picked from commit cb74ef31234d72d22a282cac6a0b18fdda6891ce) Change-Id: I0d530fc3e68a8005cf2512b2733ef36401a4af74
What changes were proposed in this pull request?
How was this patch tested?