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

Invoke-DbaQuery / Invoke-DbaAsync - Let PSObject handle more than one table #6921

Merged
merged 9 commits into from Nov 6, 2020
Merged

Invoke-DbaQuery / Invoke-DbaAsync - Let PSObject handle more than one table #6921

merged 9 commits into from Nov 6, 2020

Conversation

andreasjordan
Copy link
Contributor

Type of Change

  • Bug fix (non-breaking change, fixes # )
  • New feature (non-breaking change, adds functionality, fixes # )
  • Breaking change (effects multiple commands or functionality, fixes # )
  • Ran manual Pester test and has passed (`.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/sqlcollaborative/appveyor-lab ?
  • Nunit test is included
  • Documentation
  • Build system

Purpose

I want to get the output of sp_BlitzFirst @ExpertMode = 1 as PSObject, but the procedure returns 7 tables and -As PSObject only returns the content of the first table.

Approach

If there are more than one table, return an array of rows.

Commands to test

Install-DbaFirstResponderKit -SqlInstance SRV1\SQL2017
$BlitzFirst = Invoke-DbaQuery -SqlInstance SRV1\SQL2017 -Query 'sp_BlitzFirst @ExpertMode = 1' -As PSObject
$BlitzFirst | ForEach-Object -Process { $_ | Format-Table }
$BlitzFirst | ConvertTo-Json | Set-Content -Path C:\BlitzFirst.txt

There are maybe better approaches, so feedback is highly welcome.

@niphlod
Copy link
Contributor

niphlod commented Oct 21, 2020

did not forget this, sorry, had a busy few days. will review in a few hours

@potatoqualitee
Copy link
Member

Thank you, @andreasjordan and @niphlod ! Will wait for approval 🙇

@niphlod
Copy link
Contributor

niphlod commented Oct 22, 2020

give it a whirl, I added tests . I believe the ones failing are due to other pending changes in development

@niphlod
Copy link
Contributor

niphlod commented Oct 22, 2020

Let's forget sp_blitz for the sake of "keeping it simple".

assuming this query
image

Keeping convertto-json as the "formatter", the original version, which does an array of rows (which are arrays themselves), ends up with

[
    {
        "value":  [
                      {
                          "a":  1
                      },
                      {
                          "a":  2
                      }
                  ],
        "Count":  2
    },
    {
        "value":  [
                      {
                          "b":  2,
                          "c":  3
                      },
                      {
                          "b":  4,
                          "c":  5
                      }
                  ],
        "Count":  2
    }
]

I feel the flexibility of PSObject is hurting here, because what you can have (basically the fix I put in) is this instead

[
    {
        "a":  1
    },
    {
        "a":  2
    },
    {
        "b":  2,
        "c":  3
    },
    {
        "b":  4,
        "c":  5
    }
]

which is more akin of what you get with a single resultset, or invoking the two queries separated.
Basically the problem here is .... with a Dataset, or Datatable, the "rich" object given back has distinct tables that you can "loop" separately, each one with a fixed set of columns and one or multiple rows.

What should we make the PSObject look like for results which are not consistent (i.e. two rows have only the property "a" and the next two ones have properties "b" and "c" ) ?
Powershell likes to spit out PSObjects as they come, irregardless of the type.

Spitting arrays of arrays has the added bonus of "delimiting" what is a single resultset from another one, i.e. we can identify where one starts and ends.
But the added con of being different from the result you'd get from a single resultset.
If the "columns" between resultsets do change, you can still identify where a resultset starts and ends.

Maybe @potatoqualitee, @wsmelton and @FriedrichWeinmann have a type in mind that fits the bill ?

tl;dr: I'm fine either way (but this behaviour is new and should be fixed in stone by a regression test when we agree). I feel there are both pros and cons to each one and both implementations are coming from a solid background.... it's just that PSObject is too damn flexible that can accomodate both :D

edit:

note from @andreasjordan : We can also add a new -As ArrayOfPSObject or -As PSObjectArray

note by me : if we can't find a way to make both cohexist it sounds a good solution to the problem of "choosing"

@andreasjordan
Copy link
Contributor Author

I have thought about it again and hereby propose the new type PSObjectTable. What do you say, @niphlod ?

@niphlod
Copy link
Contributor

niphlod commented Nov 4, 2020

yeah, I don't think anything is going to cut it without an additional -As type. I'd go for PSObjectArray.

@andreasjordan
Copy link
Contributor Author

yeah, I don't think anything is going to cut it without an additional -As type. I'd go for PSObjectArray.

Yes, as it is an Array - let's call it an Array. I had the analogy with DataSet, DataTable and DataRow in mind. But I fully agree with Array.

I can change the code probably later today.

@andreasjordan
Copy link
Contributor Author

OK, updated the code, @niphlod please have a look.

@niphlod
Copy link
Contributor

niphlod commented Nov 4, 2020

the array part looks good, but -As PSObject needs to still spits out results from the two resultsets combined (as the version I posted before we embarked on a "let's make the array output optional").
Fix that, add a test with the behaviour of PSObject with multiple resultsets (this is a "new" feature and I'd like to fix the behaviour in stone with tests) and it's all perfect for me.

@andreasjordan
Copy link
Contributor Author

Like this?

Copy link
Contributor

@niphlod niphlod left a comment

Choose a reason for hiding this comment

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

nailed it! thanks for the patience

@potatoqualitee
Copy link
Member

@niphlod so sorry to bother but it's giving issues with formatting. help?

@niphlod
Copy link
Contributor

niphlod commented Nov 6, 2020

was looking but @andreasjordan beat me to it. if it does fila again hail me.

BTW: @potatoqualitee after the last version of invoke-dbaformatter which checks that a correct version of PSSA is available I can't see why yours generates a different output

@andreasjordan
Copy link
Contributor Author

Ok, all green now...

@potatoqualitee
Copy link
Member

sweet as heck, merging! thanks everyone 🙇

@potatoqualitee potatoqualitee merged commit c86a006 into dataplat:development Nov 6, 2020
@andreasjordan andreasjordan deleted the InvokeDbaQuery_Improve_AsPSObject branch November 6, 2020 15:01
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.

None yet

3 participants