-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve '*' when formatting a 'SELECT *' statement #2473
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One semi-nit inline. To clarify, this doesn't actually fix the semantics of '*' - its just deferring the final evaluation to the executor.
@@ -78,6 +96,10 @@ public Expression getExpression() { | |||
return expression; | |||
} | |||
|
|||
public Optional<AllColumns> getSource() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this from getSource to something else (maybe getAllColumns) ? It's confusing because source generally refers to a data source (stream/table) in the parser/engine. Same for other usage of the word source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @rodesai. source refers to the '*' column only, so it makes more sense to rename this method (perhaps the variable name as well) to one that relates to it, like starColumn, allColumns, whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good. Just the minor feedback about the StatementRewriter.getSource() method name.
@@ -78,6 +96,10 @@ public Expression getExpression() { | |||
return expression; | |||
} | |||
|
|||
public Optional<AllColumns> getSource() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @rodesai. source refers to the '*' column only, so it makes more sense to rename this method (perhaps the variable name as well) to one that relates to it, like starColumn, allColumns, whatever.
Description
Consider stream
X
with fieldsA
andB
. Previously, the following semantics would hold:The new behavior replaces all of the select items that came from a
*
with*
, for example:Motivation
In order to support #2436, we need to make sure that
SELECT *
statements do not expand to include ROWKEY/ROWTIME. Fixing the root cause of this behavior is tricky and perhaps backwards incompatible (see discussion #2436 (comment)), so in the meantime I propose the behavior in this PR, which I think is good independent of that PR.Testing done
New unit tests.
Reviewer checklist