Skip to content

SelectParser: add support for grouping labels#36887

Open
radu-gheorghe wants to merge 1 commit into
masterfrom
radu/select_parser_add_label
Open

SelectParser: add support for grouping labels#36887
radu-gheorghe wants to merge 1 commit into
masterfrom
radu/select_parser_add_label

Conversation

@radu-gheorghe

Copy link
Copy Markdown
Contributor

The Select (JSON) syntax didn't support a way to change the label of grouping results. The YQL variant does with something like:

each(...) as(mylabel)

This commit fixes this gap. Something like this now works:

{
  "all": {
    "group": "explanation_attribute",
    "each": {
      "output": "count()",
      "label": "explanations"
    }
  }
}

Note how the label is within each. Feel free to disagree with that :) It's in the name of simplicity/reliability, because JSON grouping is translated to "normal" grouping, where as needs to go after each (IIUC). The current implementation guarantees that each is there. If we did it one level higher (which is more natural), we'd have to do more validation to make sure we don't put as in the wrong place.

Speaking of validation, we do a reasonable job (e.g., reject the request if label is upper in the hierarchy - see tests for more), but we don't cover everything. AFAIK, the only corner case is when someone has an array for each (which is unlikely, IMO):

    "each": [{
      "output": "count()",
      "label": "explanations"
    }]

In this case, we silently drop the label instead of rejecting the request as invalid. I think this is in line with the rather loose validation that we do for JSON grouping in general: we rely on the underlying grouping syntax parser to catch some of the errors. You can argue that's not nice, but refactoring for better JSON validation is a separate task, IMO.

@boeker boeker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me 🙂

@johansolbakken

Copy link
Copy Markdown
Contributor

Looks good to me 👍🏻 , but should we run it by the architecture meeting as well?

@radu-gheorghe

Copy link
Copy Markdown
Contributor Author

Ah, that would be a good idea 🙈 I'll do it later.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants