Repository navigation
Spanner table.read() returns a list of column name-value pairs. Why not an object? #27
Description
Activity
Another clash of idiomatic vs API "what-ifs". Call
.toJSON()on a row to get the most JS-like thing we could do.I lost access to the repos we developed in, so I can't link to the history of the decision.
I believe it was mostly because columns aren't always named.
Solution: don't include them in the default response, make them available in the
apiResponseargument
Problem: The default response shouldn't exclude dataFrom @vkedia on March 17, 2017 16:43
Yes the issue is that columns might not have names or there might be
duplicate names. So it's not guaranteed that each spanner row can be
converted to a json object.On Mar 17, 2017 9:38 AM, "Stephen Sawchuk" [email protected] wrote:
I believe it was mostly because columns aren't always named.
Solution: don't include them in the default response, make them available
in the apiResponse argument
Problem: The default response shouldn't exclude data—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
googleapis/google-cloud-node#2103 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ATdef_xv90p1bfpgznGfIObanj7jOIcDks5rmrcZgaJpZM4MgxAX
.From @jgeewax on March 17, 2017 17:57
The guidelines of this library have always been: make the common case easy and the advanced case possible.
In this situation, it seems that we're making the common case annoying to make the advanced case possible, which doesn't make sense to me.
I believe there are a few reasons where we may have either missing names or ambiguous results, such as expressions (
COUNT(*)) or duplicate names (due to interleaved tables). I think the right thing to do here is:-
No matter what, the results should be addressable by their position in the query. That is, the results of
SELECT a, b, c ...should always have the value ofaasrow[0]andbasrow[1], etc. -
If there is one and only one unambiguous value for a column with a name, that should be addressable by that name on the result object. That is,
SELECT SingerName FROM Singers WHERE SingerId = 1should return a row where I can get the result by addressingrow.SingerName. -
If there is ambiguity anywhere in the results (such as duplicate names), we should throw a helpful error saying:
"Sorry, you asked for $ColumnNameHere in the result, but there were multiple fields with that name. You should either use an alias in your query or address the result you want by its position."
How's that sound?
/cc @vkedia
-
From @jgeewax on March 17, 2017 18:9
/cc @stephenplusplus : I think the question then becomes about the best way to have a thing that acts partly like an array, but with a few addressable properties for things falling into item (2) above.
Yeah, this was my original solution as well. Wish I could link to the original discussion, hoping to get access back soon.
But basically:
row = { 0: 'value-1', 1: 'value-2', 2: 'value-3', // does not have a col name namedProp1: 'value-1', namedProp2: 'value-2' } for (var prop in row) { // only iterates named properties }
The hard part is how do we communicate the ambiguity error. If we throw or return it, that takes users down the non-success execution path of their code. Could we just use the docs to state "Here's how we return the response and here's why we do it" and leave it to the user to consider when building their query?
From @jgeewax on March 17, 2017 18:36
We also want:
row.forEach(function (column, i) { // i = 0, column = { name: ... , value: ...} // but we DON'T want i = namedProp1, column = value });
Regarding the ambiguity, I see a couple different cases:
-
You're specifying the columns in a query and it results in having duplicate names.
Throwing an actual error here doesn't seem like a bad idea. You know your query, you'll see it and go "oh got it", and fix the error during development.
-
You're selecting * and it results in duplicates.
Again, throwing an error here seems reasonable. You can find this during development. It's not that scary to see "you are asking for something that is not clear... you should be more clear". Again, an actionable error message here is very important.
-
You're selecting * and right now there are no duplicates, but when you add a new column later to a table it results in duplicates.
Throwing an error here is a smidge scary because it means that production code may stop working due to the schema change. Then again, you're altering the schema on your live production instance while not being specific about what you want. I think the better thing to do here is to recommend that you be specific in how you address fields (via an index) or don't duplicate names (in the global docs we do this, by suggesting you avoid the vague name of
Idand instead use specific names likeSingerId).Being totally honest here, I suspect that most people won't even think about this and will blindly take the results we have now and turn them into an object. In that case, the last value found (for a duplicate) would win, and people would go looking for why the value for their
idcolumn is wrong, not realizing that their little helper function blasted out the first one and swapped it with the second. If anything, throwing an error here is a very good idea unless Spanner gives us enough information to tell which table a field came from (but I don't believe it does).
So my vote still is for the "if you ask for a named property that is ambiguous, we throw an actionable error message saying so". We also document in the
run()method that if you're usingSELECT *, you should be careful because you may grab stuff that conflicts and that may cause an error if you ask for something by it's name and we don't know which one you mean.-
From @vkedia on March 17, 2017 18:36
So I went through the earlier discussion and @stephenplusplus had suggested the exact solution there as well. But my objection at that point was around duplicate column names. I am fine if column with duplicate names cannot be retrieved by name and we just document that fact. But I would also like to have some way for user to know those column names. So lets says columns 0 and 1 both have the name 'a'. I am ok if both of them are just addressable by index. But there should be some way for user to get the names for both these columns. It can be some sort of extra metadata property on the row object which just contains the names and types for each column.
From @jgeewax on March 17, 2017 19:16
But there should be some way for user to get the names for both these columns.
I'm confused what you mean by this. Can you explain?
2 remaining items
@jgeewax @stephenplusplus @alexander-fenster This issue should also be closed before marking the library as GA since this will cause a really big breaking change if we do decide to address this.
Current situation is that users can always calltoJsonto get a nice JS object. They will not need to write any helper snippet for this. What is the concern with that? It is also worth noticing that in the 10 months since the library was released we have not heard from any users about this being an annoyance.@vkedia If I may add my 2 cents, this is not a huge pain, but in my team, we are only interested in the JSON response, and have to write the
toJSONline in every service we have. This is not much, but it'd be even better if we didn't have to do so. I understand that there can be missing names or name collisions, but I think the only viable solution is to update your SQL queries and use aliases anyway.As for #80 , I think this is adding complexity for the majority of the users.
@WaldoJeffers thanks. Another option can be to take some option like
returnAsJsonin the read/query API which return json objects instead of row objects.As discussed: we'll take the option approach, and the default value will be to leave the value as is. When the option is enabled, it will apply
toJSONautomatically. It will be releases as a semver minor version.Reacted by WaldoJeffersGreat news!
PR sent in #94 -- please check it out!
Reacted by WaldoJeffers and Chad BrockmanReacted by WaldoJeffers, Adam Walsh and Chad Brockman
From @jgeewax on March 17, 2017 15:52
The response of calling
table.read(query)is a list of rows, but the row isn't a native object, instead it's a key-value pair-style object:Why wouldn't we match these up as a proper object? Since we don't customers end up having to write a helper-snippet just so they can pass around a useful object... That is, it'd be nice if the rows returned were...
Copied from original issue: googleapis/google-cloud-node#2103