Repository navigation
[CALCITE-7028] Implement SQL pipe syntax - #5349
Conversation
|
Some notes about behavior and implementation. In general this patch tries to adhere to GoogleSQL behavior. One thing that was hard to align with GoogleSQL, and remains in this patch, is treatment of aliases in subqueries. In GoogleSQL, I want to call attention to one design decision: the "surrogate SELECT" concept. It's kind of a hybrid of two possible designs: rewriting the parse tree, and validating & converting pipe operators directly. Rewriting the parse tree has issues with achieving the proper behavior around retention of aliases and sort-order. Validating and converting pipe operators directly could work, but I was worried about the extra code it would end up needing. The surrogate SELECT approach gets to leverage a lot of pre-existing code. |
cd0c885 to
17f4df7
Compare
|
In general we have trouble getting reviews for large PRs. I hope someone will find the time. |
It could be made smaller by restricting it to Anyway, please let me know if cutting back the scope to just |
|
in general two smaller PRs is easier than a big one for reviewers. |
17f4df7 to
c15d68a
Compare
I just cut it back to only |
|
I like pipe syntax, I will try to review this, but it may take a while. |
c15d68a to
5c32bb7
Compare
mihaibudiu
left a comment
There was a problem hiding this comment.
This is generally fine, I think I only have style comments.
I think I understand the general structure of this PR, but it's complicated. The proof is in the tests.
| /** | ||
| * Reads optional pipe operators and applies them to the query {@code e}. | ||
| * | ||
| * <p>If {@code with}, {@code e} is a WITH, or an ORDER BY of a WITH, that is |
There was a problem hiding this comment.
can this be {@code with e}?.
How can it not be a WITH if it's a with?
The period should probably be not here.
Who is "they" who can "see"?
There was a problem hiding this comment.
Reworded it to try to be more clear:
/**
* Parses zero or more pipe operators that follow the query {@code e}.
*
* <p>The flag {@code with} signifies whether {@code e} is a WITH that is not
* in parentheses, or an ORDER BY of such a WITH. If so, the operators are
* applied to the body of the WITH, so that they can reference its CTEs.
*/
| } | ||
|
|
||
| /** | ||
| * Parses a pipe WHERE operator, and applies it to the query {@code e}. |
There was a problem hiding this comment.
what does it mean to "apply it to e"?
I don't see e being modified.
Maybe it applies the filter to the result produced by e.
There was a problem hiding this comment.
It just means it returns a call whose input is e. Reworded it to be clearer:
/**
* Parses a pipe WHERE operator, and returns a SqlPipeWhere whose input
* is the query {@code e}.
*/
| throw SqlUtil.newContextException(getPos(), | ||
| RESOURCE.pipeSyntaxNotAllowed()); | ||
| } | ||
| throw SqlUtil.newContextException(getPos(), |
There was a problem hiding this comment.
if this will be done later, maybe you can add a comment.
There was a problem hiding this comment.
Ah, this is intentional, the behavior is taken from GoogleSQL. It requires parentheses in such situations. I added a comment.
| RelDataType[] argTypes = new RelDataType[2]; | ||
| argTypes[0] = type1; | ||
| final boolean isSubQuery = node2 instanceof SqlSelect; | ||
| // Not isA(SqlKind.QUERY): coercing a WITH as a sub-query gives it a type |
There was a problem hiding this comment.
I don't understand this comment.
There was a problem hiding this comment.
I just removed the comment. It was trying to say that we need to use PIPE_QUERY here rather than QUERY. I don't think it was adding much, the line of code stands on its own.
| return rowTypeCoercion(validator.getOverScope(query), body, columnIndex, targetType); | ||
| case FROM_QUERY: | ||
| case PIPE_OPERATOR: | ||
| // A query in pipe syntax is validated and converted as its surrogate |
There was a problem hiding this comment.
can you define "surrogate"?
Is this a word used in the definition of the pipe syntax?
There was a problem hiding this comment.
It was defined in SqlPipeOperator (which is now renamed to SqlPipeCall) although I moved the definition to SqlPipeQuery where the surrogateSelect() method is defined.
|
|
||
| case PIPE_INPUT: | ||
| // The input's order matters only if the operator is at the top of the | ||
| // statement. Then convert the input as top, so that its sort is kept. |
There was a problem hiding this comment.
what does "convert the input as top" mean?
There was a problem hiding this comment.
It means to set the top flag during conversion, which causes the converter to retain ORDER BY. I reworded the comment to try to be clearer:
// If the "bb.top" flag is set, it means we are converting a pipe
// operator call at the top of the statement; therefore, we need to
// also convert the input with the top flag set. (This causes it to
// retain its ordering, which would otherwise be dropped for a
// subquery.)
| final boolean keepOrder = bb.top; | ||
| SqlNode pipeInput = ((SqlPipeInput) from).getInput(); | ||
| while (pipeInput instanceof SqlWith) { | ||
| // convertWith would convert the body as the top of the statement. |
There was a problem hiding this comment.
I also don't know what the top of a statement is
There was a problem hiding this comment.
I removed this comment, I don't think it adds much.
| final List<CorrelationId> correlNames = new ArrayList<>(); | ||
| // Mapping from (correlId, originalFieldIndex) to projectedFieldIndex for aggregation | ||
| final Map<Pair<CorrelationId, Integer>, Integer> fieldMapping = new HashMap<>(); | ||
| int correlFieldCount = Integer.MAX_VALUE; |
There was a problem hiding this comment.
This is a bit ugly. Why not just use null?
There was a problem hiding this comment.
changed it to a @Nullable Integer.
| // to update correlation variable row type and field indices | ||
| // The rel of a pipe operator's input may also have trailing columns for | ||
| // sort keys (see convertNonAggregateSelectList), which the correlation | ||
| // variable lacks. Give it the rel's row type. |
There was a problem hiding this comment.
what is "it"?
What does it mean to give something to a type?
There was a problem hiding this comment.
"it" is the correlation variable, and giving it the row type means changing the type of the variable to the rel's row type. I think the code flow here could be improved, so I restructured it a bit to split the logic for aggregation and for pipe operators. They were previously back-to-back. The comment is associated with pipe operators only now, and is reworded to try to be clearer.
| } | ||
|
|
||
| /** | ||
| * Converts a query written in pipe syntax, by converting its surrogate |
There was a problem hiding this comment.
maybe this is true, but there's nothing about surrogates in this body.
In general I would not document the implementation in the javadoc, only in the body.
There was a problem hiding this comment.
I moved the comment to the body, and it's now clearer because of the rename of PipeNamespace#getSelect -> PipeNamespace#getSurrogateSelect. (which is now referred to in the body.)
5c32bb7 to
9d41e13
Compare
|
@mihaibudiu Thank you for reviewing, I have pushed various updates and replied to your comments. |
|
CI is unhappy. I plan to re-review this after you fix it. |
9d41e13 to
6645402
Compare
@mihaibudiu There was a style problem, I just fixed it. |
|
Please, you amend your commit on a 2.3KLOC PR? How am I supposed to find out what you changed? Now I have to read everything again? Maybe you can dig your history and make the changes as a separate commit. |
6645402 to
5f42088
Compare
I updated the branch to be the commit you originally reviewed + the follow up as a separate commit. I also merged in And, sorry, I had got confused about how things work around here. Originally I tried doing a second commit and it ran afoul of the LintTest. I looked around at what people were doing in other recent PRs, and generally saw force pushing happening, so I figured that it was standard here to avoid having multiple commits on a branch. Looking closer I now see that the LintTest just wants follow up commit messages to be formatted a certain way. Blame trying to get CI working at the end of the day. |
|
mihaibudiu
left a comment
There was a problem hiding this comment.
I only left one comment, but I think this is good.
If no one else objects I think we should merge this (this will require the commits to be squashed, but let's wait a bit in case someone else is reviewing).
|
From my pov you can squash the commits. |
I just replied to that comment: #5349 (comment) and pushed a small follow up commit: 2ac63b5. No main code changes, just comments and a test. |
This patch adds FROM and |> WHERE from SQL pipe syntax. The syntax is implemented in the core parser, and is available only in certain conformances. The approach is to keep the operators in the parse tree through validation, ultimately converting them to relational operators in SqlToRelConverter. This approach enables us to adhere to pipe syntax rules, especially the rules around table alias visibility and sort ordering retention. It also enables us to write pipe-syntax-aware error messages. To simplify validation and conversion, each pipe operator is associated with a surrogate SELECT that acts as its stand-in. The pipe operator itself is used for pipe-specific validations, and retaining the correct scopes and collations.
2ac63b5 to
3e9cfde
Compare
|
I have squashed the commits. |
|
I will merge this, but I won't mark [CALCITE-7028] as closed yet, since I think you are planning a follow-up. |
Thank you. I am hoping to do follow ups for all of the common pipe operators. I could do them all in CALCITE-7028 or I could raise a new Jira for each follow up. If you have a preference please let me know. Fwiw, the list I had in mind for "common pipe operators" is: SELECT (done in this PR), WHERE (done in this PR), ORDER BY, LIMIT, AGGREGATE, UNION, INTERSECT, EXCEPT, EXTEND, SET, DROP, JOIN, RENAME, AS, and DISTINCT. To avoid huge PRs it would probably need to be ~8 PRs total. |
|
Smaller PRs are easier to review. You can bundle these fixes in a few smaller PRs - it depends really how big they would turn out. |



This patch adds FROM and |> WHERE from SQL pipe syntax. The syntax is implemented in the core parser, and is available only in certain conformances.
The approach is to keep the operators in the parse tree through validation, ultimately converting them to relational operators in SqlToRelConverter. This approach enables us to adhere to pipe syntax rules, especially the rules around table alias visibility and sort ordering retention. It also enables us to write pipe-syntax-aware error messages.
To simplify validation and conversion, each pipe operator is associated with a surrogate SELECT that acts as its stand-in. The pipe operator itself is used for pipe-specific validations, and retaining the correct scopes and collations.