Skip to content

Spanner table.read() returns a list of column name-value pairs. Why not an object? #2103

Description

@jgeewax

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:

[ [ { name: 'id', value: { [Number: 1] value: '1' } } ],
 [ { name: 'id', value: { [Number: 2] value: '2' } } ],
 [ { name: 'id', value: { [Number: 3] value: '3' } } ]

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...

[ {id: '1'}, {'id': '2'}, {'id': '3'} ]

Activity

  1. stephenplusplus commented on Mar 17, 2017

    @stephenplusplus
    Contributor

    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.

  2. jgeewax commented on Mar 17, 2017

    @jgeewax
    ContributorAuthor

    @vkedia This seems... silly. What were the what-ifs so we can sort this out?

  3. stephenplusplus commented on Mar 17, 2017

    @stephenplusplus
    Contributor

    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

  4. vkedia commented on Mar 17, 2017

    @vkedia
    Contributor
  5. jgeewax commented on Mar 17, 2017

    @jgeewax
    ContributorAuthor

    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:

    1. 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 of a as row[0] and b as row[1], etc.

    2. 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 = 1 should return a row where I can get the result by addressing row.SingerName.

    3. 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

  6. jgeewax commented on Mar 17, 2017

    @jgeewax
    ContributorAuthor

    /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.

  7. stephenplusplus commented on Mar 17, 2017

    @stephenplusplus
    Contributor

    Yeah, this was my original solution as well. Wish I could link to the original discussion, hoping to get access back soon.

  8. stephenplusplus commented on Mar 17, 2017

    @stephenplusplus
    Contributor

    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?

  9. jgeewax commented on Mar 17, 2017

    @jgeewax
    ContributorAuthor

    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:

    1. 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.

    2. 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.

    3. 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 Id and instead use specific names like SingerId).

      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 id column 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 using SELECT *, 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.

  10. vkedia commented on Mar 17, 2017

    @vkedia
    Contributor

    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.

  11. jgeewax commented on Mar 17, 2017

    @jgeewax
    ContributorAuthor

    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?

  12. vkedia commented on Mar 17, 2017

    @vkedia
    Contributor

    So user should be able to know what the name of a column whose index is i is. It can be as some sort of metadata on the row object.

  13. chadbr commented on Mar 20, 2017

    @chadbr

    could the metadata only exist if there is a conflict? adding a metadata object to every row sounds really expensive...

  14. added
    priority: p2Moderately-important priority. Fix may not be included in next release.
    on Apr 19, 2017
  15. stephenplusplus commented on Nov 2, 2017

    @stephenplusplus
    Contributor

    This issue was moved to googleapis/nodejs-spanner#27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

api: spannerIssues related to the Spanner API.priority: p2Moderately-important priority. Fix may not be included in next release.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions