unify SQL and pipe name inference and simplify single by-only agg output #6263
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit unifies the way l-values are inferred for expressions that omit the LHS of a field assignment or the AS clause in a select SELECT expression. ANSI SQL does not have specific guidance on how to infer arbitrary expressions so we blended in some of the existing logic from the pipe syntax. We also changed how "this" is handled. Instead of raising an error, the l-value becomes "that". We also adapted the heuristic for a single column agg with no groupings to work also for a single grouping with no aggs (i.e., emitting the values instead of a single-column record). These two changes means that "by this" now works and behaves like distinct. These changes also means that there's more typing with operators like cut when you want to preserve the object path, e.g., cut x.y.z:=x.y.z, but less typing when you don't, e.g., cut x.y.z to mean cut z:=x.y.z. Overall, this improves the consistency and orthogonality of the language.
philrz
reviewed
Sep 23, 2025
| output: | | ||
| {y:123,"upper(s)":"HELLO, WORLD","sqrt(2)/z":2.8284271247461903} | ||
| {y:123,upper:"HELLO, WORLD","sqrt(2)/z":2.8284271247461903} |
Contributor
There was a problem hiding this comment.
I'm sure there'll always be ways to trip over this, but the loss of the (s) feels like maybe a step backwards since now it's much easier to hit this:
$ echo '{x:{y:123},s:"Hello, world", r:"bye", z:0.5}' | super -c 'values {x.y,upper(s),upper(r),sqrt(2)/z}' -
record expression: duplicate field: "upper" at line 1, column 22:
values {x.y,upper(s),upper(r),sqrt(2)/z}
~~~~~~~~
But I guess a temporary problem since we'll eventually get to #5977?
Collaborator
Author
There was a problem hiding this comment.
Yes temporary but I didn't realize the extent of the impact here.
philrz
reviewed
Sep 23, 2025
Contributor
There was a problem hiding this comment.
I assume the name of this test should change (or be deleted entirely?) since its original raison d'etre seemed to be confirming that query produced an error and as your change shows it no longer does that.
Per our discussion, I tried to change cut to take path expressions instead of assignments but this had an impact on the optimizer tests. When changing cut to values in these tests, they were no longer optimized the same way, which means the optimizer needs some work. It needed this work anyway, but rather than do this here, I backed out some of the cut changes and will defer the updates to a subsequent PR.
mattnibs
approved these changes
Sep 23, 2025
nwt
added a commit
that referenced
this pull request
Nov 17, 2025
The GET /query/describe route sends a response containing the output keys for aggregation operations but Zui is interested in the input keys, so send those instead. This wasn't a problem for Zui for most queries until #6263, which changed the output key for "aggregate by a.b" from a.b (the path of the input key) to b (the last path component of the input key).
nwt
added a commit
that referenced
this pull request
Nov 17, 2025
The GET /query/describe route sends a response containing the output keys for aggregation operations but Zui is interested in the input keys, so send those instead. This wasn't a problem for Zui for most queries until #6263, which changed the output key for "aggregate by a.b" from a.b (the path of the input key) to b (the last path component of the input key).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit unifies the way l-values are inferred for expressions that omit the LHS of a field assignment or the AS clause in a select SELECT expression. ANSI SQL does not have specific guidance on how to infer arbitrary expressions so we blended in some of the existing logic from the pipe syntax.
We also changed how "this" is handled. Instead of raising an error, the l-value becomes "that". We also adapted the heuristic for a single column agg with no groupings to work also for a single grouping with no aggs (i.e., emitting the values instead of a single-column record). These two changes means that "by this" now works and behaves like distinct.
Overall, this improves the consistency and orthogonality of the language.
Fixes #6157
Fixes #5315