-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Wildcard expansion #3628
Wildcard expansion #3628
Conversation
d7225ef
to
5f0169b
Compare
@@ -953,6 +953,31 @@ func (s *SelectStatement) HasWildcard() bool { | |||
return false | |||
} | |||
|
|||
// HasFieldWildcard returns whether or not the select statement has at least 1 wildcard in the fields | |||
func (s *SelectStatement) HasFieldWildcard() bool { |
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.
Since these two functions now exist, how about transforming HasWildcard
into
return HasFieldWildcard() || HasDimensionWildcard()
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.
Yeah, I haven't reviewed any of this code yet or refactored (hence the WIP in the title). Makes sense.
OK, if this change meets the requirements of @pauldix, + 1 from me. |
5f0169b
to
6656483
Compare
@@ -181,6 +182,11 @@ func (lm *LocalMapper) Open() error { | |||
selectTags.add(tsf.selectTags...) | |||
whereFields.add(tsf.whereFields...) | |||
|
|||
// If we only have tags in our select clause we just return | |||
if len(selectFields) == 0 && len(selectTags) > 0 { |
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.
I presume this shouldn't actually be an error.
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.
That's a good question. It feels like maybe this should be an error? Right now you'll just get an empty result set, but maybe we should say "You need at least one field in your select statement". That may change in the future of course based on features we add. Would you prefer an error over an empty result set in this scenario?
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.
Yeah, I think it should be an error, but these cases are no longer simple due to the distributed nature of the system. I would be OK with opening a 0.9.4 ticket, and coming back to this.
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.
Perhaps just return an error for now. It's a small change, so let's do it.
Some questions, but +1. |
+1 |
d6266a1
to
4e04c06
Compare
Addresses the raw query portion of #3549