Fixes #1632

[yuce]

Overview

[Describe what this pull request addresses.]

Fixes #1632

Pull request checklist

• I have read the contributing guide.
• I have agreed to the Contributor License Agreement.
• I have updated the documentation.
• I have resolved any merge conflicts.
• I have included tests that cover my changes.
• All new and existing tests pass.

Code review checklist

This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.

• Ensure that any changes to external docs have been included in this pull request.
• If the changes require that minor/major versions need to be updated, tag the PR appropriately.
• Ensure the new code is properly commented and follows Idiomatic Go.
• Check that tests have been written and that they cover the new functionality.
• Run tests and ensure they pass.
• Build and run the code, performing any applicable integration testing.

[travisturner]

This looks good, but there are some issues when column keys are used.

1. executor.translateCall() needs to contain SetColumnAttrs like this:

if c.Name == "Set" || c.Name == "Clear" || c.Name == "Row" || c.Name == "SetColumnAttrs" {

This is a separate bug, but it causes problems with testing this PR. It's related to #1679. In fact, we should probably audit that method to ensure that all possible query types are handling the translate calls correctly there.

2. with keys, the output ends up looking like this:

  "columnAttrs": [
    {
      "id": 0,
      "key": "one",
      "attrs": {
        "name": "To Kill a Mockingbird",
        "year": 1960
      }
    },
    {
      "id": 0,
      "key": "two",
      "attrs": {
        "name": "No Name in the Street",
        "year": 1972
      }
    }
    }
  ]

It would be nice if we could suppress that "id": 0, from the output when a key is present. It seems like we've done that elsewhere.

3. We should add a test that covers the columnAttr key translation.

[travisturner]

resp.ColumnAttrSets needs to be columnAttrSets

[yuce]

Wouldn't that break compatibility with Pilosa 1.0 ?

[travisturner]

I mean that this range loop is using the wrong argument.
resp.ColumnAttrSets is empty, but columnAttrSets was populated in the code block above.
The range needs to loop over columnAttrSets.

[travisturner]

this line should be

if idx.Keys() {

[yuce]

Updated

[travisturner]

I definitely like the idea of cleaning this up, but let's make it a switch statement instead of a map lookup:

switch c.Name {
case: "Set", "Clear", "Row", "SetColumnAttrs":
    ...
case: "SetRowAttrs":
    ...
default:
    ...
}

[travisturner]

@yuce I just merged #1679 which actually implements this switch, so you can just rebase master for the update, and add in "SetColumnAttrs".

[yuce]

Updated

[travisturner]

I'm not sure if this works for the case where ID is actually supposed to be 0 (with no column key). Should we open a ticket to write a custom JSON marshaler?

[yuce]

Why do we try to remove ID from the result in the first place ?

[travisturner]

Well, we omit the key when it doesn't apply, so it seems to make sense to omit the ID when it doesn't apply. And when there is a key, the ID is always 0, so it's not really useful anyway. I don't have a strong opinion on this, however. I'll make a ticket suggesting we revisit that output and it can be discussed there.