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

Support array input parameters #714

Closed
msullivan opened this issue Apr 14, 2022 · 7 comments · Fixed by #1200
Closed

Support array input parameters #714

msullivan opened this issue Apr 14, 2022 · 7 comments · Fixed by #1200
Assignees
Labels
bug Something isn't working

Comments

@msullivan
Copy link
Member

If I try to specify an array input param in the CLI, I get

Unimplemented input type descriptor: Array(ArrayTypeDescriptor { id: 4cf81264-f02d-46d6-ff1d-9f46c355fb3e, type_pos: TypePos(0), dimensions: [None] })
@tailhook tailhook transferred this issue from edgedb/edgedb-rust Apr 18, 2022
@tailhook tailhook added the minor Can be postponed arbitrarily label Apr 18, 2022
@msullivan msullivan added bug Something isn't working minor Can be postponed arbitrarily and removed minor Can be postponed arbitrarily labels Sep 1, 2022
@jackfischer
Copy link

Would question this being minor. Arrays are common and we often copy/paste queries with param from QB into the CLI.

@msullivan msullivan removed the minor Can be postponed arbitrarily label Mar 6, 2023
@msullivan
Copy link
Member Author

@tailhook could you take a look at this soon? Tuples, also.

Last year I hacked up something that basically worked (228cec1) but felt like it might be too hacky even by my standards, which think are lower than @tailhook's

So it might require a more substantial rework of input parsing

@msullivan
Copy link
Member Author

There is one potential actual UI design question, which is how to syntactically represent arrays of str, given that plain strs are just entered in without quotes or anything, but I think the only really reasonable answer is "quoted using edgeql's lexical conventions"

@tailhook
Copy link
Contributor

tailhook commented Mar 7, 2023

I don't expect that I have time for this in 3.0 timeframe. This might be introduced in 3.x cycle.

There is one potential actual UI design question, which is how to syntactically represent arrays of str, given that plain strs are just entered in without quotes or anything, but I think the only really reasonable answer is "quoted using edgeql's lexical conventions"

For testing queries, I've expected something along the lines of:

> SELECT <array<str>>$arr
Variable $arr[0]: element1
Variable $arr[1]: element2
Variable $arr[2]: <Ctrl+D>

But if people use arrays in REPL a lot, it will not be very convenient. Also I wanted to be able to switch between input modes via a keyboard shortcut. But rustlyline we use, don't work well with custom shortcuts.

@msullivan
Copy link
Member Author

One difficulty with the multi-entry approach is that if we have <optional array<str>>$0, we would struggle to distinguish between empty set and empty array.

@quinchs quinchs linked a pull request Jan 31, 2024 that will close this issue
msullivan added a commit that referenced this issue May 23, 2024
This PR reworks the variable input mechanism to use nom as a parser; allowing us to parse array inputs and other complex inputs.

Arrays and tuples are parsed using edgeql syntactic conventions. Inside of arrays and tuples,
strings must be quoted using edgeql conventions.

A good follow-up might be to support parsing things like uuids as `<uuid>'acb53d04-184e-11ef-aaab-cb2f1138c5d7'` and similar, as well as using more abbreviated syntax.

Fixes #714

---------

Co-authored-by: Michael J. Sullivan <sully@msully.net>
@jackfischer
Copy link

Occurs to me on another reading that "minor" might have referred to implementation difficulty! Sorry for the presumptuous comment

@msullivan
Copy link
Member Author

Occurs to me on another reading that "minor" might have referred to implementation difficulty! Sorry for the presumptuous comment

No, your interpretation was right as was (in my opinion at least) your disagreement with tagging it "minor".
The main reason it took so long to get fixed was that the implementation actually was pretty involved, since
the whole input parsing system needed to be redone in a way that supported composition of the parsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants