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

Improve PowerShell experience for working with databases #11604

Closed
SteveL-MSFT opened this issue Jan 16, 2020 · 23 comments
Closed

Improve PowerShell experience for working with databases #11604

SteveL-MSFT opened this issue Jan 16, 2020 · 23 comments
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Issue-Enhancement the issue is more of a feature request than a bug Resolution-No Activity Issue has had no activity for 6 months or more

Comments

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 16, 2020

Summary of the new feature/enhancement

Initially for 7.0, we accepted a change to have [DBNull]::Value treated as $null. The expectation is that this would make it easier for DB users where their content may contain a DBNull and not require them to explicit check that as well as $null. However, this introduced other issues:

#11084
#10980
#10404

(note that we should probably treat NullString and AutomationNull equivalently)

So we had to revert that change before 7.0 GA (note: that change was reverted in rc.2 release branch which will be used for GA release, but kept in the master branch for now). We would still like to investigate whether such a change (or perhaps a different change) makes sense and consider all peripheral impact (so not just -eq, but -lt, etc... as well as how it works with null coalescing, etc...)

@SteveL-MSFT SteveL-MSFT added Issue-Enhancement the issue is more of a feature request than a bug Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif labels Jan 16, 2020
@vexx32
Copy link
Collaborator

vexx32 commented Jan 16, 2020

@SteveL-MSFT NullString and AutomationNull should be treated equivalently? [nullstring]::Value | % { 1 } should return no results?

Just want to clarify, because I don't think that point has been brought up before -- I recall @daxian-dbw mentioning we wanted it to behave the same way [dbnull]::Value does though.

@SeeminglyScience
Copy link
Collaborator

I know I'm a bit late to say this but they definitely want this right? I don't know much about databases, but is there really no reason to check for DBNull?

/cc @potatoqualitee @SQLDBAWithABeard

@vexx32
Copy link
Collaborator

vexx32 commented Jan 17, 2020

There does need to be a way to explicitly test for DBNull in some circumstances I'm sure, but from what I've heard the difference is mostly a pain and complicates conditional expressions unnecessarily.

If you need to check for DBNull specifically you could still do so with -is [dbnull] and I think it makes sense for that to be preserved even if we otherwise treat DBNull the same as $null for most / all other intents and purposes.

We should also reference the original feature request here: #10404

Tangentially related is this issue about treatment of AutomationNull as well, I suppose: #9997

@ConstantineK
Copy link

My read on the original issue is that DBNull makes working in idiomatic powershell and SQL Server harder for noobs, you dont have to do any special magic to get value types out of your data normally. A dbnull requires an entirely different set of conditionals and checks before you get to anything resembling your business logic.

Exactly @vexx32

@potatoqualitee
Copy link

+1 to CK. It's ultra confusing initially (and I still don't understand the underlying mechanics). I've had to visit this SO post more than a couple times in my sql ps travels.

@vexx32
Copy link
Collaborator

vexx32 commented Jan 17, 2020

I'm sure if we were redesigning these APIs these days it would be much simpler to always have the same wrapper return type that indicated both 1) was something returned (true normally, and false would indicate what we currently get as null from those methods iirc?), and 2) what the resulting value is; in most cases you'd just look at the value (which would simply be null both in the case that there is a null value in the DB field and also if there is no data returned at all). In the rare case that you do want to check if the database even had anything to give you'd check the other field.

I think this solution is likely the closest we're likely to get to that more friendly user experience for handling database values at present, at least until the database APIs get replaced with a newer library entirely (which likely won't happen for quite a few years yet).

The hard part is, as @SteveL-MSFT mentioned in the original post, there are some edge cases we'd need to account for when doing this special handling, and we want to be sure we're handling it appropriately in those cases as well. 🙂

@SQLDBAWithABeard
Copy link

SQLDBAWithABeard commented Jan 17, 2020

OK, this is a tricky one I think.

In database terms NULL has a specific meaning, different in my understanding, to the meaning in a programming language. The absence of a value instead of the absence of an object or object property. (you can dive deeper on SQL here https://www.red-gate.com/simple-talk/sql/t-sql-programming/how-to-get-nulls-horribly-wrong-in-sql-server/)

You cannot compare to a NULL value, two null values are not equal in a database. In SQL we use IS (NOT) NULL, which matches obviously to DbNull here.

So as the two have a different meaning if I do something like this

CREATE TABLE HasNUlls (id INT,somenullshere varchar(3))

INSERT INTO HasNUlls VALUES 
(1,NULL),
(2,'1'),
(3,'5'),
(4,'115'),
(5,NULL)

and ask the database to return the somenullshere column where ID = 5 (Where it is a database NULL) and where ID = 6 (where it doesnt exist) I get a different response.

image

and the same in pwsh

image

and they are not the same :-)

image

which is what is required otherwise you are hiding the difference between the two and perpetuating the impression that DbNull -eq $null

I might do something like this

$Query = "SELECT * FROM HasNulls"
$AllOfThem = Invoke-DbaQuery -SqlInstance $sqlinstance -Database tempdb -Query $query
$dbnull = [System.DBNull]::Value
$AllOfThem | Where somenullshere -eq $null
$AllOfThem | Where somenullshere -eq $dbnull

image

The last command might be returning the SQL Instances that my information gathering scripts have not gathered any information for. (To be honest this is a made up example with flaws as I would probably use SQL to do the data munching here as that is what it is good for)

The first command might help me to find any SQL Instances that I have not got in my reference table.These are two different things.

The complication comes when you dont know what you are returning and need another comparison (as @vexx32 says) but personally, I think people need to code for the two options.

This I think is how it should be for -lt -gt etc comparisons as well.

I would like

$AllOfThem | Where somenullshere -eq [System.DBNull]::Value

to work without first setting it to a variable and if there was a $DatabaseNull variable that would be even better (but naming it would be hard to avoid breaking things (like my scripts!))

Simple Talk
NULLs in SQL are a mixed blessing. The Three-Valued Logic of SQL has its uses but can cause difficulties to anyone who assumes that NULL has a 'value'. It can make reports go horribly wrong in a number of alarming ways, as Robert Sheldon explains.

@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Jan 17, 2020

I would like

$AllOfThem | Where somenullshere -eq [System.DBNull]::Value

to work without first setting it to a variable and if there was a $DatabaseNull variable that would be even better (but naming it would be hard to avoid breaking things (like my scripts!))

You can do -eq ([DBNull]::Value) or Where { $_.somenullshere -eq [DBNull]::Value }. As much as I'd love to be able to use type literals and static members in command arguments, that's currently valid syntax for a bare word string (e.g. echo [DBNull]::Value works like echo '[DBNull]::Value')

But also keep in mind that with the changes proposed, you'd have to change that to Where somenullshere -is ([DBNull]), otherwise it would also include all literal null values (correct me if I'm wrong @vexx32).

@vexx32
Copy link
Collaborator

vexx32 commented Jan 17, 2020

Yeah that's correct @SeeminglyScience.

The proposal is not that they be considered entirely identical to $null in PS, but moreso that PS will allow you to somewhat ignore the difference when the distinction isn't important, much as it allows coercion for many other types when the distinction isn't especially important. -eq and most other comparison operators are well known to do type coercions between quite a few different types, and this would let you easily compare values without always needing to first check for DBNull if you don't particularly care about the underlying difference in the database state.

When distinguishing the true states is imperative, you would still be able to use $value -is [dbnull] to be absolutely certain 🙂

@SQLDBAWithABeard
Copy link

@SeeminglyScience I do know that! I was being a bit daft because we're are discussing making it simpler 😉

@vexx32 I think understanding when the difference is not important is a difficult thing

@vexx32
Copy link
Collaborator

vexx32 commented Jan 17, 2020

From the other folx responses, it seems that understanding the reason for the difference at all is a bit baffling at the best of times. If @potatoqualitee hasn't quite mastered it, I'm not sure there's a lot of hope for the rest of folx out there at the moment! 😆

But yes, that's a valid concern -- I suppose the question is, is it better to be able to ignore the difference, or should we be exposing the difference as we currently are, and forcing users to deal with it themselves? From what I've seen, the current way it's handled leads to a lot of boilerplate and messy -or conditionals when folx don't really care one way or the other.

I definitely can't speak to what would be a better route, though; that's why we're pinging all you folx left and right. 💖 😁

@SteveL-MSFT
Copy link
Member Author

In the case of getting a null value vs getting null returned as in no results, that's no different today in just PowerShell:

PS> (1,2,3,$null).count
4
PS> $null.count
0

@vexx32, regarding nullstring and automationnull, let's defer whether they should be symmetric to dbnull until after we resolve whether a change makes sense and what kind of change is needed for dbnull

@SeeminglyScience
Copy link
Collaborator

The thing I keep coming back to is does this really solve the original problem? In the first thread that spawned @vexx32's PR (#9561), it was mentioned that dbnull keeps escaping into non database code. That's still going to happen, just not for comparisons specifically. The dbnull is still going to be there, so as soon as you pass it to something that doesn't work like PowerShell (a dotnet method, a serializer, etc) won't this just make the problem harder to understand?

I'm not saying that the above will or won't be a problem necessarily, I just want to make sure it's been considered.

@vexx32
Copy link
Collaborator

vexx32 commented Jan 17, 2020

Very good point. I'm sure we could make adjustments to the method binder to handle DBNull as well, but equally... should we? 🤔 😕

@SeeminglyScience
Copy link
Collaborator

Very good point. I'm sure we could make adjustments to the method binder to handle DBNull as well, but equally... should we? 🤔 😕

Would there be scenarios where you would want to actually send dbnull to a method though? You'd have to have a RealDBNull type like NullString which would get really confusing. On top of that, you might have to recursively search objects for members that are DBNull, same with arrays (which even AutomationNull doesn't handle, but is more likely to occur here).

It may be the case that most of the time the comparison change is fine. Maybe most affected code doesn't actually end with sending the objects somewhere that would cause any issues. I'm not sure if this can really be answered without testing, so this might just need to be put into a beta and tested vigorously by the DBA folks.

@essentialexch
Copy link

I wasn't thrilled about the original change (making DBNull compare like $null), perhaps because I had already gone through the learning curve so that it didn't impact my scripts.

But I agree with the reversion and think it should stay reverted. Different values should have different comparisons IMO. Sometimes, a higher learning curve is associated with accuracy.

@daxian-dbw
Copy link
Member

@SteveL-MSFT We probably need a decision on whether to revert those 2 PRs in the master branch too. New changes may touch the same code that were changed in those 2 PRs and make a reversion harder.

@SteveL-MSFT
Copy link
Member Author

@daxian-dbw we should revert those changes in master until we have some conclusion in this issue

@FireInWinter
Copy link

I really hope this reversion isn't permanent. The difference between $null and DBNull.Value is just semantics for the most part. It's like saying 3 x infinity is greater than 2 x infinity. It's sort of a true statement depending on the type of math your using, but also meaningless in almost all cases. I've worked with SQL Server, MySQL, and other databases for years. I understand the difference, but personally I wouldn't mind if DBNull.Value was replaced by $null entirely. It's greatly simplifies usage in PowerShell and if you know the datasource, you can understand the origin of the null. And if you don't know the origin of the data, you won't understand the DBNull anyway. Heck, I had to write a function Remove-DBNull to replace DBNull with null in result sets. It sucks since it slows down usage of the results, but it makes it much easier to use. If it at least compared as $null, I probably wouldn't bother with converting the data unless I was exporting it to another system.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

2 similar comments
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 16, 2023
Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Issue-Enhancement the issue is more of a feature request than a bug Resolution-No Activity Issue has had no activity for 6 months or more
Projects
None yet
Development

No branches or pull requests

9 participants