Skip to content

DRILL-7604: Allow session options to be set in HTTP queries#1997

Closed
dobesv wants to merge 1 commit intoapache:masterfrom
dobesv:DRILL-7604/http-session-options
Closed

DRILL-7604: Allow session options to be set in HTTP queries#1997
dobesv wants to merge 1 commit intoapache:masterfrom
dobesv:DRILL-7604/http-session-options

Conversation

@dobesv
Copy link
Contributor

@dobesv dobesv commented Feb 25, 2020

DRILL-7604: Allow session options to be set in HTTP queries

Description

Normally session options must be set using the SET command. However,
this command does not work for HTTP clients (REST API and Web UI)
because it doesn't keep a session between requests.

This changes it so that a request can contain the options you might want
to set using the SET command. It also updates the Web UI to include a
widget to select the store.format option so users can specify the file
format that will be used be CREATE TABLE AS. Additional options could
be added later if desired without much difficulty.

Documentation

POST /query.json

Submit a query and return results.

Parameters

  • queryType--SQL, PHYSICAL, or LOGICAL are valid types. Use only "SQL". Other types are for internal use only.
  • query--A SQL query that runs in Drill.
  • autoLimit--Limits the number of rows returned from the result set. (Drill 1.16+)
  • options--Object containing session options that you could have set using the SET command. Note that numeric & boolean
    options should be provided as a number or boolean, not as a string. A common iuse case would be to set "store.format":"json"
    to specify that CREATE TABLE AS should output in JSON format.

Testing

I added a JUnit test. I also did some manual testing.

(Please describe how this PR has been tested.)

@dobesv
Copy link
Contributor Author

dobesv commented Feb 26, 2020

The test pass on my machine, does anyone have any hint on how I could try to reproduce the failure?

Copy link
Contributor

@paul-rogers paul-rogers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful addition to the REST API. One suggestion about simplifying the code, another about the list of CREATE TABLE formats.

@arina-ielchiieva
Copy link
Member

@dobesv also please notice that new tests fail on GitHub Actions, please make sure tests pass.

@dobesv
Copy link
Contributor Author

dobesv commented Feb 26, 2020

I don't know how to fix the tests, I need some help with that.

@dobesv dobesv force-pushed the DRILL-7604/http-session-options branch from 8b749de to 6de0e50 Compare February 26, 2020 20:36
Copy link
Contributor

@paul-rogers paul-rogers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the improvement and the thorough tests. Just a few nits this time around.

@dobesv dobesv force-pushed the DRILL-7604/http-session-options branch 2 times, most recently from b0c41f2 to ebcd2ea Compare February 27, 2020 05:29
Copy link
Contributor

@paul-rogers paul-rogers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dobesv, thanks for the PR. We can always make it better, but I'll be mindful of your time.

Oh, realized we still have that crash:

Errors: 
  TestQueryWrapper.testShowSchemas:106->run:75->runWithOptions:95 » NullPointer
 TestQueryWrapper.testSpecifyOutputFormatAsJson:131->runWithOption:101->runWithOptions:95 » NullPointer
TestQueryWrapper.testSpecifyOutputFormatAsParquet:115->runWithOption:101->runWithOptions:95 » NullPointer

I'll see if I can poke around and find the cause. Once we fix that, we'll be good to go.

@dobesv
Copy link
Contributor Author

dobesv commented Feb 27, 2020

@paul-rogers Thanks Paul, I appreciate your help. I'm not experienced with the project, and I'm quite out of date with my Java skills, so hopefully this is something you could debug in minutes whereas it could take me many hours.

@dobesv dobesv force-pushed the DRILL-7604/http-session-options branch from ebcd2ea to 984b5bb Compare March 1, 2020 02:00
@dobesv dobesv force-pushed the DRILL-7604/http-session-options branch from 984b5bb to ef1b875 Compare March 10, 2020 21:35
@arina-ielchiieva
Copy link
Member

@dobesv how user will set session options through web ui?

@dobesv
Copy link
Contributor Author

dobesv commented Mar 11, 2020

This PR doesn't include a way to set options via the Web UI any more, I guess that'll be something for a future PR.

If we come up with a list of options that are popular to set then we could add those in follow-up PRs. I had an example of such a field in this PR originally but it turned out be problematic. I don't have a good replacement example option in mind currently.

Normally session options must be set using the `SET` command.
However, this command does not work for HTTP clients (REST API and Web
UI) because it doesn't keep a session between requests.

This changes it so that a request can contain the options you might
want to set using the `SET` command.  It also updates the Web UI to
include a widget to select the `store.format` option so users can
specify the file format that will be used be `CREATE TABLE AS`.
Additional options could be added later if desired without much
difficulty.
@dobesv dobesv force-pushed the DRILL-7604/http-session-options branch from a178a59 to b73c7a0 Compare March 11, 2020 16:33
@arina-ielchiieva
Copy link
Member

@dobesv what about having empty box where user can set options?

@dobesv
Copy link
Contributor Author

dobesv commented Mar 11, 2020

@arina-ielchiieva It sounds a bit complicated, maybe this PR can proceed without it and we can look into adding a new UI for setting options in the future.

@arina-ielchiieva
Copy link
Member

@dobesv the problem is that I don't really like parsing of the options introduced in this PR (its partially duplicates work done in Drill plus does parsing in slightly different way which will lead to discrepancies in future), there is much cleaner way to parse options, you need to pass option name, its kind and value. Please refer to my previous comment https://github.com/apache/drill/pull/1997/files#r391122758 so this PR needs re-working.

@dobesv
Copy link
Contributor Author

dobesv commented Mar 12, 2020

OK, I'll just set this aside for now.

@dobesv dobesv closed this Mar 12, 2020
@paul-rogers
Copy link
Contributor

@dobesv, this is a super useful feature. Perhaps I can tackle the Drill-specific bits that @arina-ielchiieva requested. How would you like do this? Should I issue a PR against your branch? Clone this branch and push a new PR?

@dobesv
Copy link
Contributor Author

dobesv commented Mar 15, 2020

@paul-rogers Feel free to make a new branch from this one and carry on from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments