Skip to content
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

Make commands pluggable #114

Closed
vlsi opened this issue Aug 20, 2018 · 17 comments
Closed

Make commands pluggable #114

vlsi opened this issue Aug 20, 2018 · 17 comments

Comments

@vlsi
Copy link

vlsi commented Aug 20, 2018

I'm exploring the possibility to use SQLLine for headless mode in vlsi/mat-calcite-plugin#9

The use-case there is as follows:

UC1) Start the application in command line mode, and use interactive console. In interactive mode, it would be nice to have mat-calcite-specific commands like !open-dump cool_dump.hprof.

The thing is !open-dump command does not make much sense in SQLLine without mat-calcite.

UC2) "human-friendly" way to configure Calcite connection. For instance: attach two heap dumps as two Calcite schemas. It might be a good idea to attach JDBC connection to yet another schema as well.
It would be nice to be able to "attach/detach" DB connections with simple !add-connection schema_name jdbc:postgresql:... or something like that.
This is more Calcite-specific than mat-clacite-specific, however it does not look like SQLLine-specific either.

@snuyanzin
Copy link
Collaborator

snuyanzin commented Aug 20, 2018

UC1) Currently there is command !connect e.g.

!connect jdbc:calcite:model=target/test-classes/wiki.json admin admin

why not use something similar for dumps for instance (the only issue here as I could see is it will ask you for username/password) but it requires a jdbc driver which knows how to work with mat:calcite:plugin

!connect mat:calcite:plugin=cool_dump.hprof 

UC2) May be I do not understand your point but currently there is such possibility to have several connections like

!connect jdbc:calcite:model=example/csv/target/test-classes/smart.json admin admin
!connect jdbc:calcite:model=example/csv/target/test-classes/bug.json admin admin

and to see the list of active like

3: jdbc:calcite:model=example/csv/target/test> !list
4 active connections:
 #0  open                          jdbc:calcite:model=example/csv/target/test-classes/model.json
 #1  open                          jdbc:calcite:model=example/csv/target/test-classes/model.json
 #2  open                          jdbc:calcite:model=example/csv/target/test-classes/bug.json
 #3  open                          jdbc:calcite:model=example/csv/target/test-classes/smart.json

also there is !go command to switch between active connections and !all command to run query against all active connections

@vlsi
Copy link
Author

vlsi commented Aug 20, 2018

why not use something similar for dumps for instance

  1. User experience. At the end of the day, I don't want everybody get full understanding the way SQLLine, Calcite and mat-calcite internals. The simpler the command the better. Well, !connect mat:calcite:plugin=cool_dump.hprof is not that bad, however !open_dump cool_dump.hprof would be even better. !connect cool_heap.hprof could be fine as well.

  2. Autocomplete. !open-heap could suggest .hprof files, and it could produce better error messages.

  3. I might want to hide some commands as well. For instance, importedkeys does not make much sense for my use case, and dropall does look like a disaster waiting to happen.

also there is !go command to switch between active connections and !all command to run query against all active connections

I want to fetch information from the database for the objects identified in a heapdump. So it would be nice to be able to write a single SQL that refers to both "heapdump" tables and "DB tables".

@snuyanzin
Copy link
Collaborator

snuyanzin commented Aug 20, 2018

I'm not 100% sure but it looks like moving to Jline3 and commands as widgets will allow to achieve it. It is possible to remove existing and add new (on the fly as well). Currently I try to move to Jline3 and could check if widgets help or not by the end of the week in my sandbox

@julianhyde
Copy link
Owner

I like the idea of making commands pluggable. (I did it recently in Quidem, which is a similar tool in many respects, and was happy with the result.)

I don't see a reason to couple pluggable commands from migration to jline3. Commands are more of a 'back-end' thing.

@vlsi
Copy link
Author

vlsi commented Aug 21, 2018

I don't see a reason to couple pluggable commands from migration to jline3

Is jline3 welcome at all?
It looks like trying jline3 first, and implementing pluggable commands later would make more sense than the other way around.

@julianhyde
Copy link
Owner

julianhyde commented Aug 21, 2018 via email

@snuyanzin
Copy link
Collaborator

snuyanzin commented Aug 23, 2018

Thank you for pointing to quidem. I looked at widgets more accurate and unfortunately it looks like that they are too impecunious for this task (e.g. no completer, help text support)

At the same time I did the similar to quidem way. Now it's possible to add custom commands like

sqlline version 1.5.0-SNAPSHOT
sqlline> !hello
Unknown command: hello
sqlline> !commandHandler sqlline.MyAggregatedCommandHandler
sqlline> !hello
HELLO WORLD
sqlline> !quit

or via -ch flag for while sqlline.sh,sqlline.bat call
Also please have a look at test sqlline.SqlLineArgsTest#testCommandHandler at [1]. I would like to get some feedback for current version and to understand if it is acceptable not only plug custom commands but also unplug them and at least some of embedded.

However at the moment it does not allow to hide existing functions like !dropall, !importkeys and others. What do you think of idea to split existing commands at least into 2 separate groups with an aggregated command handler for each. But one of them is required (including !manual, !set, !quit,!brief,!verbose and etc) while the other is optional (including !dropall, !importedkeys and etc,) and could be turned off for instance

sqlline version 1.5.0-SNAPSHOT
sqlline> !commandHandler turnoff sqlline.OptionalCommandHandler
sqlline> !dropall
Unknown command: dropall

[1] master...snuyanzin:SQLLINE_114#diff-553a413667266842ab8cd7d1faadfbd8R742

@arina-ielchiieva
Copy link
Collaborator

I am not sure that two handlers with achieve my main goal with hiding commands. I think there should be an option for the user to hide commands he does not need.

@snuyanzin
Copy link
Collaborator

Thank you for your response.
Agree may be 2 is not a good option but it is just an example of grouping.

The issue (and above there was an attempt to cope with it) is to allow to hide commands but at the same time to have a limited set of commands which are not allowed to be hidden (otherwise I can not imagine how to handle situations with hidden !commandHandler, !quit, !connect for instance).

I agree it would be better to hide by command than by command handler and did changes to make it possible (also the limitation to 2 command handlers is removed):

  1. For hiding commands there is !hide which can hide one or more (also it could hide itself). Help and completers info will also be hidden for hidden commands
sqlline> !hide tables history
sqlline> !tables
Unknown command: tables
sqlline> !history
Unknown command: history
  1. For adding new commands !commandHandler is used
sqlline> !hello
Unknown command: hello
sqlline> !commandHandler sqlline.HelloWorldCommandHandler
sqlline> !hello
HELLO WORLD

What is still unclear to me: is it ok to allow a user to hide any command e.g. !quit ?

The current version could be seen at
master...snuyanzin:SQLLINE_114

@arina-ielchiieva
Copy link
Collaborator

I agree that we should not allow to hide all commands. Commands like quit should be always present.
Also how we'll be able to pass hide on startup, i.e. --hide=cmd1,cmd2?

@snuyanzin
Copy link
Collaborator

snuyanzin commented Aug 27, 2018

Yes that is right.
Now added support of hide on startup. Also added docs update and tests including " on startup", e.g.

sqlline -hide scan,outputformat

about commands which are not allowed to hide the current set is
!quit, !help, !manual, !history, !verbose, !brief, !set

master...snuyanzin:SQLLINE_114

@julianhyde
Copy link
Owner

Let's not do the -hide option. That kind of customization is for developers (by writing java code) not for end-users. I think we should use the Application class proposed in #106.

@snuyanzin
Copy link
Collaborator

In this case I have a question: how to resolve situation if end-users want to "override" command (add a command handler with existing command name)?
Current solution does not allow it.
Should we allow overriding with some warn messages or just prohibit overriding for end-users?

@julianhyde
Copy link
Owner

These "end-users" would be java programmers (since they know how to write a command handler). I claim that they are not typical end-users. We can make them write a sub-class of Application.

@snuyanzin
Copy link
Collaborator

ok, i understand, thank you.
removed -hide option at #119

@snuyanzin
Copy link
Collaborator

With current solution it is possible to specify one or more command handlers like

!commandHandler <commandHandler class name> [<commandHandler class name>]*

does it make sense to have possibility to specify *.jar and add all command handlers from this jar? Something similar to beeline add

@julianhyde
Copy link
Owner

Fixed in 5a6789d, PR #119. Thanks @snuyanzin!

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

No branches or pull requests

4 participants