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

database/sql: add generic Null[T] #60370

Closed
methane opened this issue May 23, 2023 · 29 comments
Closed

database/sql: add generic Null[T] #60370

methane opened this issue May 23, 2023 · 29 comments

Comments

@methane
Copy link
Contributor

methane commented May 23, 2023

database/sql doesn't have NullUInt64. So drivers and ORMs may implement their own NullUInt64.
Application developers would be confused which NullUInt64 is returned by dynamic Scan methods.

When NullUInt64 was proposed in past, @a8m, @kardianos, @rsc said "wait for generic".
#47953 (comment)

Now we have generics. So can we have it for now? Should it Null[T] or Nullable[T]?

Here is quick implementation: https://go.dev/play/p/Jtx8hP0H1_Z

@gopherbot gopherbot added this to the Proposal milestone May 23, 2023
@methane methane changed the title proposal: database/sql: Add generic Nullable[T] proposal: database/sql: Add generic Nullable[T] May 23, 2023
@seankhliao
Copy link
Member

see also #48702

@rsc rsc changed the title proposal: database/sql: Add generic Nullable[T] proposal: database/sql: add generic Null[T] May 24, 2023
@rsc rsc changed the title proposal: database/sql: add generic Null[T] proposal: database/sql: add generic Null[T] May 24, 2023
@rsc
Copy link
Contributor

rsc commented May 24, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 31, 2023

/cc @kardianos @bradfitz

@rsc
Copy link
Contributor

rsc commented Jun 7, 2023

Looking at the existing NullInt64, etc they name the value field after the type (Int64 for NullInt64). We can't do that for the generic version - it needs a single field name for all the instances. Value is the obvious choice but that has to be a method. Val is the next obvious choice but then the fields are Val and Valid. Maybe X and Valid?

@rsc
Copy link
Contributor

rsc commented Jun 7, 2023

Does someone want to write a CL and see how it looks?

@a8m
Copy link
Contributor

a8m commented Jun 7, 2023

Maybe X and Valid?

I suggest going with V and Valid, and don’t mind changing my CL to support this.

but that has to be a method

Could you please elaborate on why it needs to be a method?

@ianlancetaylor
Copy link
Contributor

Value has to be a method in order to implement database/sql/driver.Valuer.

methane added a commit to methane/go that referenced this issue Jun 8, 2023
Generic version of NullString, NullInt64, etc.

Fix golang#60370
@methane
Copy link
Contributor Author

methane commented Jun 8, 2023

I sent pull request with X and Valid.
It has only test copied from NullString. I will add other tests if needed.

@gopherbot
Copy link

Change https://go.dev/cl/501700 mentions this issue: database/sql: add Null[T]

@mateusz834
Copy link
Member

I feel like Nullable[T] is a better name for this.

@leaxoy
Copy link

leaxoy commented Jun 10, 2023

I think Null[T] is more useful not only in database/sql, it's a more general abstraction used to describe has a value or nothing there, and should defined outside database/sql.

This abstraction in rust/scala is Option and in haskell is Maybe, all of them use sum type to construct it.

There are many proposals to add sum type to go, latest is #57644. Maybe we can wait it make progress.

@methane
Copy link
Contributor Author

methane commented Jun 11, 2023

We have waited for generics. Do you saying that we should wait for sum type again?

@leaxoy
Copy link

leaxoy commented Jun 11, 2023

@methane Not exactly, the Null[T] you proposed in other language is the concept of fp. And not only database/sql depend on it, there are many uses in other places too. For example, when indexing a slice/array or get value from map, Null[T] can indicate result is either some of T or nothing.
Should we move to a separate package like maybe or optional.

@bradfitz
Copy link
Contributor

The database/sql Scan value type is not going to be a general option type that non-SQL users use. Make your own type elsewhere if that's what you want. But even then it won't be widely used by all the libraries so it'll always feel awkward and not idiomatic in Go.

@rsc
Copy link
Contributor

rsc commented Jun 14, 2023

It should be Null[T] to match NullString etc. If those were named NullableString then we'd use Nullable[T]. This is only about sql.

@rsc
Copy link
Contributor

rsc commented Jun 14, 2023

Have all concerns about this proposal been addressed?

@rsc
Copy link
Contributor

rsc commented Jun 21, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 28, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: database/sql: add generic Null[T] database/sql: add generic Null[T] Jun 28, 2023
@rsc rsc modified the milestones: Proposal, Backlog Jun 28, 2023
@jba
Copy link
Contributor

jba commented Jul 28, 2023

http://go.dev/cl/c/go/+/501700 missed go 1.21, but it should be able to get into 1.22. I made some comments on the CL.

We still haven't picked a name for the value field. I know that go/ast uses X a lot, but at least there it might mean "eXpression." Here it just feels odd. How about Val?

@jba
Copy link
Contributor

jba commented Jul 28, 2023

I missed Russ's point about Val and Valid. Maybe V then?

@jba
Copy link
Contributor

jba commented Aug 11, 2023

http://go.dev/cl/c/go/+/501700 has been merged. This will be in 1.22.
@methane, as the author of the CL, would you like to add something to the release notes?

@gopherbot
Copy link

Change https://go.dev/cl/519555 mentions this issue: doc/go1.22: mention new sql.Null[T]

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Aug 16, 2023
gopherbot pushed a commit that referenced this issue Aug 16, 2023
For #60370.

Change-Id: Idae906ec7027be6d95f78bf43f7ce8f9d07e6c00
GitHub-Last-Rev: c645f0c
GitHub-Pull-Request: #62033
Reviewed-on: https://go-review.googlesource.com/c/go/+/519555
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@LukaGiorgadze
Copy link

Before it arrives:

here's the implementation of go null with generics: https://github.com/lomsa-dev/gonull
supports custom types.

package main

import (
	"encoding/json"
	"fmt"

	"github.com/lomsa-dev/gonull"
)

type MyCustomInt int
type MyCustomFloat32 float32

type Person struct {
	Name    string
	Age     gonull.Nullable[MyCustomInt]
	Address gonull.Nullable[string]
	Height  gonull.Nullable[MyCustomFloat32]
}

func main() {
	jsonData := []byte(`{"Name":"Alice","Age":15,"Address":null,"Height":null}`)

	var person Person
	err := json.Unmarshal(jsonData, &person)
	if err != nil {
		panic(err)
	}
	fmt.Printf("Unmarshalled Person: %+v\n", person)

	marshalledData, err := json.Marshal(person)
	if err != nil {
		panic(err)
	}
	fmt.Printf("Marshalled JSON: %s\n", string(marshalledData))
}

@allochi
Copy link

allochi commented Sep 11, 2023

I'm sorry that my comment comes late to this proposal, but I do agree with @leaxoy that Null[T] use goes beyond just database/sql package, and it would be nice if it's somewhere else like optional package maybe.

@dolmen
Copy link
Contributor

dolmen commented Oct 17, 2023

@leaxoy @allochi We are discussing here about a type that has to implement interfaces database/sql.Scanner and database/sql/driver.Valuer. This is very specific to database/sql.

@arvenil
Copy link

arvenil commented Apr 8, 2024

One thing is not entirely clear for me, is there a point to use e.g. *NullString once you can use Null[T]? Wouldn't it be more clear for new comers if the old Null types were marked as obsolete?

@jba
Copy link
Contributor

jba commented Apr 9, 2024

I don't think there is any use for NullString over Null[string].
It's a separate question whether NullString and related types should be deprecated.

@DeedleFake
Copy link

Would it break anything for NullString to be declared as type NullString = Null[string]? It would still be a struct { string; bool } with the same methods. Embedding, maybe?

@jba
Copy link
Contributor

jba commented Apr 9, 2024

It would break reflect.TypeOf(NullString{}) != reflect.TypeOf(Null[string]{}).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.