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.

Sign up for GitHub

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

Jump to bottom

Accept pipeline input for stor insert values #11433

Open
NotTheDr01ds opened this issue Dec 28, 2023 · 9 comments · Fixed by #12882
Open

Accept pipeline input for stor insert values #11433

NotTheDr01ds opened this issue Dec 28, 2023 · 9 comments · Fixed by #12882
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone
v0.95.0

Comments

@NotTheDr01ds
Copy link
Contributor

NotTheDr01ds commented Dec 28, 2023 •
edited

Related problem

While it's been established that into sqlite's ability to append to a database is more a "side-effect" than part of its design, it seems so much simpler than stor insert. For example, with a REST call that returns a JSON items with a list of records (i.e. a Nu table):

With into sqlite:

let res = (http get $endpoint)
$res.items
    | select colname1? colname2? colname5?
    | into sqlite foo.sqlite

This currently works when both creating the database as well as updating, as long as the values match their original positions.

With stor insert:

let res = (http get $endpoint)
$res.items
    | each {
        |item|
        stor insert -t main -d {
            colname1: $item.colname1,
            colname2: $item.colname2,
            colname5: $item.colname5
        }
    }

(^ from memory - Hope I got those right)

Describe the solution you'd like

Allow stor insert to accept pipeline input in the form of either a record:

{bool1: true, int1: 5, float1: 1.1, str1: fdncred, datetime1: 2023-04-17} | stor insert --table-name nudb
$table | select bool1? float1? str1? datetime1? | stor insert --table-name nudb

In the second case, I'm guessing the column names would need to be inferred from the first record, which into sqlite seems to do when creating (but not updating) the database (to support streaming). Any change in the signature of future records would result in an error.

Describe alternatives you've considered

No response

Additional context and details

Would this also work for stor update?
@NotTheDr01ds NotTheDr01ds added enhancement New feature or request needs-triage An issue that hasn't had any proper look labels Dec 28, 2023
@fdncred
Copy link
Collaborator

fdncred commented Dec 28, 2023

I'd support that. It should be pretty easy to do. You just have to check for input and use it and make sure you don't have input and --data-record parameters at the same time.

I'm all for making these stor commands as ergonomic as possible.

@fdncred fdncred removed the needs-triage An issue that hasn't had any proper look label Dec 28, 2023
@JoaoFidalgo1403
Copy link
Contributor

Hi, me and my colleague would like to know if it is still possible to work on this enhancement!

@fdncred
Copy link
Collaborator

fdncred commented Apr 25, 2024

@JoaoFidalgo1403 Sure. I was just thinking that it could work both ways. If the -d parameter is they, work the way I wrote it. If the -d parameter is missing and there is input being passed in as a record, then use that, but like I said, if you have both input and -d, that's an error case.

@fdncred fdncred added the good first issue Good for newcomers label Apr 25, 2024
@friaes
Copy link
Contributor

friaes commented Apr 25, 2024

@fdncred Also, would it be ok if we did the same work on stor update ?

@fdncred
Copy link
Collaborator

fdncred commented Apr 25, 2024

@fdncred Also, would it be ok if we did the same work on stor update ?

i think so

@bobby-palmer
Copy link 

        let columns: Option<Record> = call.get_flag(engine_state, stack, "data-record")?;
        match _input {
            PipelineData::Empty => {
                 // do nothing
            },
            PipelineData::Value(v, _) => {
                if columns.is_some() {
                    //error
                }
                else {
                    columns = Some(v.into_record()?);
                }
            },
            _ => {
                   // error
            }
        }

Would something like this be on the right track? I'm new to the code base and a little unfamiliar with the code style.
@fdncred

@fdncred
Copy link
Collaborator

fdncred commented May 13, 2024

@bobby-palmer I think that's probably part of it, although input shouldn't have an underscore if it's being used. If there's no input, you should look for the data as a parameter. You might dig through the command to see which ones can work with input and parameters. I don't have any in mind right now.

@bobby-palmer bobby-palmer mentioned this issue May 13, 2024
Closed
@JoaoFidalgo1403 JoaoFidalgo1403 mentioned this issue May 16, 2024
Merged
fdncred pushed a commit that referenced this issue Jun 6, 2024
@JoaoFidalgo1403 @friaes
Allow stor insert and stor update to accept pipeline input (#12882)
073d885 
- this PR should close #11433 

# Description
This PR implements pipeline input support for the stor insert and stor
update commands,
enabling users to directly pass data to these commands without relying
solely on flag parameters.

Previously, it was only possible to specify the record data using flag
parameters,
which could be less intuitive and become cumbersome:

```bash
stor insert --table-name nudb --data-record {bool1: true, int1: 5, float1: 1.1, str1: fdncred, datetime1: 2023-04-17}
stor update --table-name nudb --update-record {str1: nushell datetime1: 2020-04-17}
```

Now it is also possible to pass a record through pipeline input:

```bash
{bool1: true, int1: 5, float1: 1.1, str1: fdncred, datetime1: 2023-04-17} | stor insert --table-name nudb
{str1: nushell datetime1: 2020-04-17} | stor update --table-name nudb"
```

Changes made on code:

- Modified stor insert and stor update to accept a record from the
pipeline.
- Added logic to handle data from the pipeline record.
- Implemented an error case to prevent simultaneous data input from both
pipeline and flag.

# User-facing changes

Returns an error when both ways of inserting data are used.

The examples for both commands were updated and in each command, when
the -d or -u fags are being used at the same time as input is being
passed through the pipeline, it returns an error:


![image](https://github.com/nushell/nushell/assets/120738170/c5b15c1b-716a-4df4-95e8-3bca8f7ae224)

Also returns an error when both of them are missing:


![image](https://github.com/nushell/nushell/assets/120738170/47f538ab-79f1-4fcc-9c62-d7a7d60f86a1)


# Tests + Formating
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

Co-authored-by: Rodrigo Friães <rodrigo.friaes@tecnico.ulisboa.pt>
@hustcer hustcer added this to the v0.95.0 milestone Jun 6, 2024
@NotTheDr01ds
Copy link
Contributor Author

NotTheDr01ds commented Jun 7, 2024 •
edited

@fdncred Can you reopen? As noted in the PR, it only implements passing a record into stor insert, but not a list. As you can see in the first example, this issue was mainly about lists, with records being secondary (since that's what stor insert was using in the first place).

@fdncred fdncred reopened this Jun 7, 2024
@fdncred
Copy link
Collaborator

fdncred commented Jun 7, 2024

I reopened it but it's pretty confusing to me what the difference is. Maybe there should be a separate issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
v0.95.0
6 participants
@fdncred @hustcer @NotTheDr01ds @bobby-palmer @friaes @JoaoFidalgo1403