nushell
Proposal: Remove the sqlparser SQLite commands
What?
This PR removes the functionality to build up SQL queries using sqlparser and the Nu commands which correspond 1-to-1 with SQL clauses.
Specifically, it removes the following 20 commands: and, as, collect, describe, field, fn, from table, group-by, into db, into nu, join, limit, open-db, or (for expressions), or (for queries), order-by, over, select, testing-db, where
Please comment if you disagree with this proposal; nothing is set in stone, this is a breaking change and open to discussion (as always) .
Why?
These commands were a very cool experiment! But they serve a very niche use case, and they often conflict with existing Nu command names (using command overloading). In the interest of paring down Nu's core functionality, I think it is time to thank these commands for their service and say goodbye.
Deets / Context
Nu currently has 3 ways to query a SQLite database:
1. With standard Nu commands like get:
open foo.db | get some_table | sort-by some_column
2. With a SQL query in string form:
open foo.db | query db "SELECT * FROM some_table ORDER BY some_column"
3. With Nu commands that map 1-to-1 with SQL clauses and overload based on the input type:
open foo.db | into db | select * | from table some_table | order-by some_column
This PR only removes 3; the other 2 methods of querying a SQLite database are still present. I believe that 1 and 2 are sufficient for nearly all use cases.
I found 3 to be fundamentally awkward given that it involves writing 1 language (SQL) in another (Nu). A user needs to know SQL and how the Nu commands map to SQL. IMO 3 adds complexity instead of removing it in most cases. 3 also complicates documentation for some of Nu's most fundamental commands:
I'm kind of conflicted with this PR. I may be the only one.
- I don't use many of the sqlite commands at all, until I need them.
- When I need sqlite, I usually don't use the commands you're removing here. I tend to write sql.
- I agree that this will cleanup some of the overloaded command problems, make things smaller, compile faster, etc.
On the flip side A. This functionality is just cool and demonstrates to others some interesting things that can be done with nushell. B. I hate to flush any commands, never to be seen again. I'd prefer to have a plugin with all this functionality, so it's at least not lost and could be used or studied if someone wanted to do something similar. C. I'd also want @elferherrera's input because I believe he spent many hours implementing this stuff. I'm betting he won't have a problem.
And for the Future I would like to expand sqlite functionality in such a way that when people need to use it, nushell can provide a complete solution. I'd like nushell to provide whatever that is so people don't need to go to a gui. I'm not sure what that looks like but is more of a goal.
B. I hate to flush any commands, never to be seen again. I'd prefer to have a plugin with all this functionality, so it's at least not lost and could be used or studied if someone wanted to do something similar.
A plugin would be nice but I don't have the energy to move all of this to a plugin and maintain it. If someone else does that would be very cool.
Either way, the code will live on in Git history forever.
I am a fan of holding off and NOT deleting these commands quite yet...
We have a lot going on at the moment with lots of moving parts changing and if it is not URGENT to delete them now --- I think we need some time to come up with an SQL strategy moving forward...
We have several options which I would prefer to iron out first prior to the deletion.
@rgwood I know you are working on different things at the moment and I know this is yet another item on your plate...
But here are some ways we can move forward with SQL over the next couple of months or so...
All 3 options use sqlparser
- Datafusion which incorporates both the sql piece and dataframes
- https://github.com/gluesql/gluesql uses sled as its back end but we could also possibly use parquet / etc... or output to nu tables
- prql also aligns with these ideas
More on all of these topics in a different document that specifically outlines an SQL strategy... I was starting to write them down here but this is not the right spot...
In closing --- @rgwood if you really feel that we need to delete them now and have a good reason to do it based on your lack of bandwidth or excitement to maintain this code moving forward ---- we might first want to see if someone in the community would be willing to take on this code /
That might be too much --- I don't know... thoughts ?
Also I think this code could be valuable for giving us the ability to understand how to move to the other 3 mentioned strategies above...
@rgwood what ever decision you all make I am comfortable with just thought I would throw my 2 cents into the rink...
curious to hear what @elferherrera thinks as well... 😄
@rgwood
I changed my mind and went ahead and reviewed further ---- and agree with you that they should be deleted 👍
I used the commands when we were first working on this stuff at the beginning of your initial project with nushell when you first got started...
And this was some of your first work / If I am remembering back in time...
But not since then...
I think what you are saying makes sense ---- the cool thing about this code is that it uses the sqlparser code base which if we solve the problem a different way including datafusion uses the sqlparser...
But that is not a good enough reason to keep the code around....
Oh well --- its a good lesson in life for being able to let go of stuff... Including your own code... 😄
You are correct it did serve a purpose and in the end the purpose was to elucidate that its not the correct approach...
@rgwood were you planning on deleting this code after the release or before ?
edited : Just saw your comment on discord and it looks like you were thinking of waiting till after the release which makes sense to me...
Definitely after. I'd like to give people more time to discuss this, and changes shortly before a release are always a little scary.
At first glance, it seems scary and crazy. But after looking into your Context, I'd put 1 vote for land this :-D
I'm going to take a whirl at moving these into a plugin. It should be an interesting test.