Skip to content
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

Merged
merged 11 commits into from
Aug 14, 2015
Merged

Wildcard expansion #3628

merged 11 commits into from
Aug 14, 2015

Conversation

corylanou
Copy link
Contributor

Addresses the raw query portion of #3549

  • Add more integration test scenarios

@@ -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 {
Copy link
Contributor

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()

Copy link
Contributor Author

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.

@otoolep
Copy link
Contributor

otoolep commented Aug 12, 2015

OK, if this change meets the requirements of @pauldix, + 1 from me.

@@ -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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

@otoolep
Copy link
Contributor

otoolep commented Aug 14, 2015

Some questions, but +1.

@pauldix
Copy link
Member

pauldix commented Aug 14, 2015

+1

@corylanou corylanou changed the title WIP - Wildcard expansion Wildcard expansion Aug 14, 2015
corylanou added a commit that referenced this pull request Aug 14, 2015
@corylanou corylanou merged commit 014812d into master Aug 14, 2015
@corylanou corylanou deleted the wildcard-expansion branch August 14, 2015 20:29
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.

None yet

3 participants