CASSGO-36 could map scan get nil instead of zero value for null value

[OleksiienkoMykyta]

Closes #1699

This PR adds a new feature (MapScanWithNullableValues) that allows users to receive nullable values from Cassandra.

[jameshartig]

Based on this being possible outside of the package I'm hesitant to approve this. See my comment on the original issue: #1699 (comment)

[OleksiienkoMykyta]

@jameshartig I believe we should consider implementing features directly within gocql rather than requiring external solutions. It would be much more convenient to have these features available out of the box. Furthermore, it seems like this behavior should be the default since Cassandra can return nullable values. Currently, gocql represents these as default Go values, which alters the original data. To address this, we should at least provide an alternative functionality that preserves the original data and aligns with Cassandra's behavior.

[joao-r-reis]

So it looks like @jameshartig is proposing a change to the behavior of MapScan so it fixes this issue without adding a new separate method for it if I'm understanding correctly?

With #1856 we wouldn't need this PR (CASSGO-36) right?

[joao-r-reis]

Or is #1856 just limited to byte slices and we still can't read nullable values for other types?

[jameshartig]

Or is #1856 just limited to byte slices and we still can't read nullable values for other types?

Yes, that's just for byte slices. The reason being that nil slices are equivalent to empty slices and there's no need to make an empty slice if we don't need to.

Furthermore, it seems like this behavior should be the default since Cassandra can return nullable values.

This is maybe something we can consider for 2.0 but there's no way we could make that change in the existing version.

Currently, gocql represents these as default Go values, which alters the original data.

You can use pointers if you which to get the "original data". I've seen places that utilize the default Go values as a feature because they don't have to check for nil and can rely on the default value. I also provided a code example in the original issue with how you can do this quite easily without any changes to gocql.

[joao-r-reis]

This is maybe something we can consider for 2.0 but there's no way we could make that change in the existing version.

The release that we're working on atm (main branch) is 2.0 but it might be too intrusive of a change, I'd prefer adding a method with this capability and deprecating the current one (or not deprecating at all and just having both but that might not make much sense I'm not sure)

[joao-r-reis]

So it looks like there's no consensus on whether CASSGO-36 should actually be considered, maybe this needs a [discuss] thread on the C* dev mailing list?

[OleksiienkoMykyta]

It seems like misbehavior that MapScan doesn't get the original data from the database. Sure, we can use Scan and make a custom "MapScan" as suggested in #1699 (comment), but it looks like overhead, and as mentioned in the issue, it's confusing, but it is still confusing, especially for new users, and imo MapScan doc is not clear enough when we're talking about null values. I think there is no issue with backward compatibility because this functionality will be added to 2.0.
Also, I'm a little confused about the argument that this could be implemented on the user side, for example, SliceMap, can be easily implemented by the user, but we still provide such convenient functionality.

[jameshartig]

It seems like misbehavior that MapScan doesn't get the original data from the database.

I agree with @joao-r-reis, I think we need more consensus that "original data" is not the default go values and is actually nil. Again, I've seen cases where one or the other are preferred. I don't think we can make this large of a change based on 1-2 cases. I don't even know how many devs are using MapScan outside of exoteric situations.

imo MapScan doc is not clear enough when we're talking about null values.

We can definitely clarify the docs.

I think there is no issue with backward compatibility because this functionality will be added to 2.0.

You're changing a public interface which means anything implementing TypeInfo will need to be updated to support the new behavior.

Also, I'm a little confused about the argument that this could be implemented on the user side, for example, SliceMap, can be easily implemented by the user, but we still provide such convenient functionality.

I can appreciate your frustration but we want to decrease the surface area of the API and not increase it. We're stuck with SliceMap now but I agree it should have been more scrutinized.

[joao-r-reis]

I think I misunderstood this issue because reading this comment #1785 (comment) made me believe that we actually have no way of differentiating between a NULL and empty value from the database. It looks like there is indeed a way of doing this even if it requires a bit of extra code, correct? If this is the case then I'm starting to lean with @jameshartig here and CASSGO-36 (+ this PR) should probably just be closed

[jameshartig]

I think I found a way of achieving this without any changes to public interfaces. That said, I'm concerned about breaking existing code. My goal is that users can upgrade to 2.0 easily and anything that's broken the compiler will complain about (method changes, new arguments, etc). If we changed MapScan to instead return nil instead of default values, the compiler wouldn't pick up on that and it could subtly break code by causing a panic.

[joao-r-reis]

If we changed MapScan to instead return nil instead of default values, the compiler wouldn't pick up on that and it could subtly break code by causing a panic.

Yeah that doesn't sound good, I'd prefer to add a new method and deprecate the current one (while keeping it around for backwards compatibility) to make it clearer for users (which is kind of what this PR is currently doing already).

Either add a new method for map scan that properly returns nil values and deprecate the current one until people have the chance to transition or just keep both methods long term (I'd prefer deprecating the current one)

[OleksiienkoMykyta]

@jameshartig @joao-r-reis So what did we come up with? I like Joao's Idea to add a new method and sign the old one as deprecated. It will eliminate the backward compatibility issue and provide the proper behavior.
If it's what we are looking for, I can start to work on it.
Also, I found one more mention or related issue, looks like the is a need to add this.
#1248

[jameshartig]

@OleksiienkoMykyta I've been busy with #1855 and I completely changed how map scan works. I'd like to get that in a good place and then get back to this as it should be significantly easier now. I also haven't had enough time to think through the best way for this and I've been looking at other DB libraries for inspiration.

[jameshartig]

I'd like to deprecate SliceMap, MapScan, RowData. The existing MapScan is neat that it lets you pass in an existing map, but there's a call out to always pass in a new map and it replaces the existing values with non-pointers but accepts pointers. Seems a lot cleaner to just return a new map with all of the values, and any null columns would be nil values in the map. Something like Values() (map[string]interface{}, bool). The name Values lends itself to a []any to match Columns so ValuesMap might be more appropriate. I could also see just Map. I don't really like the bool but it could be confusing to return empty map if there's no row. Either way you'd need to do if ok or if len(map) > 0.

[OleksiienkoMykyta]

@jameshartig so, are we just waiting for #1855 to be merged for now?

[joao-r-reis]

Seems a lot cleaner to just return a new map with all of the values

I wonder if some users prefer passing in an existing map so it can be reused (GC/mem optimization).

[jameshartig]

I wonder if some users prefer passing in an existing map so it can be reused (GC/mem optimization).

Maybe but would we need to delete keys from the map that didn't match any results? Maybe not, but it still seems like a weird API.

@jameshartig so, are we just waiting for #1855 to be merged for now?

Yes, and sorry for taking so long on that.