Skip to content

Fix Impossible Query to account for Group By clauses#2409

Merged
sougou merged 1 commit intovitessio:masterfrom
HubSpot:fix_impossible_group_by
Jan 3, 2017
Merged

Fix Impossible Query to account for Group By clauses#2409
sougou merged 1 commit intovitessio:masterfrom
HubSpot:fix_impossible_group_by

Conversation

@bbeaudreault
Copy link
Copy Markdown
Contributor

We had a query which uses an aggregation and group by, and our mysql uses sql_mode=only_full_group_by. The impossible query generated by vitess did not include the group by clause, and so the query violated the conditions of only_full_group_by:

In aggregated query without GROUP BY, expression #1 of SELECT list contains nonaggregated column 'Foo.sr.id'; this is incompatible with sql_mode=only_full_group_by

This PR does three things:

  • Include the GroupBy in the select query, if the GroupBy is not nil
  • Refactors the code path so that both the vttablet and vtgate use the same impossible query generation code.
  • Update all tests to expect a group by in the generated field queries

In terms of refactoring, creating a new file in the sqlparser packaged seemed cleanest. I have not created new tests for this file, because it seems to be covered by the existing planbuilder and route tests. But if you'd like me to add new tests, I can do so.

Note: my CLA should be ready some time this week, waiting on legal department

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part can be simplified. How about something like this instead:

formatter := func(buf *sqlparser.TrackedBuffer, node sqlparser.SQLNode) {
  switch node := node.(type) {
  case *sqlparser.ColName:
    // like before
  case * sqlparser.TableName:
    // like before
  }
  // Call impossible query formatter instead of node.Format.
  sqlparser.ImpossibleQueryFormatter(buf, node)
}

Then, all you need to do is implement ImpossibleQueryFormatter in the parser.

In the case of vttablet, you can directly instantiate sqlparser.NewTrackedBuffer(sqlparser.ImpossibleQueryFormatter).

@bbeaudreault
Copy link
Copy Markdown
Contributor Author

FYI my Google CLA has been signed by the VP of Engineering at my company. Company name is HubSpot, Inc

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Dec 28, 2016

@michael-berlin has limited availability. We can have him check on this when he comes back.

@bbeaudreault
Copy link
Copy Markdown
Contributor Author

Looks like our CLA was confirmed, I got an email and see it active at https://cla.developers.google.com/clas. Thanks @michael-berlin!

I'll be wrapping up this PR soon, have been stuck doing some other things. Thanks for the comments.

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@bbeaudreault bbeaudreault force-pushed the fix_impossible_group_by branch from a774def to 0913e4c Compare January 3, 2017 18:22
@bbeaudreault
Copy link
Copy Markdown
Contributor Author

@sougou simplified

When a group by is available, ensure it's part of field query to avoid issues with aggregations
Refactor to use a unified code path between vtgate and vttablet
@bbeaudreault bbeaudreault force-pushed the fix_impossible_group_by branch from 0913e4c to baedf97 Compare January 3, 2017 19:41
@sougou sougou merged commit 2a12d48 into vitessio:master Jan 3, 2017
@bbeaudreault
Copy link
Copy Markdown
Contributor Author

thanks!

@bbeaudreault bbeaudreault deleted the fix_impossible_group_by branch January 3, 2017 21:58
dispalt added a commit to dispalt/vitess that referenced this pull request Jan 9, 2017
* 'master' of https://github.com/youtube/vitess: (220 commits)
  vtgate/vindexes: Do not run testfiles.Locate() from init() (2nd time).
  Add comment to document when new fields are included in response
  publish site Fri Jan  6 08:02:30 PST 2017
  Updating doc.
  Optionally send additional C API fields through in responses
  Fixing explorer to handle empty files.
  MultiRow Insert Optimization (vitessio#2422)
  Now the local examples use 'zk2' topology.
  Adding Consul 0.7.2 to base docker image.
  etcd2topo: using lease ID in lock filenames.
  Implementing the Consul topology client.
  test: vtgate_buffer.py: Add second test for vtctl TabletExternallyReparented.
  test: vtgate_buffer.py: Stop the the two threads in case of a test error.
  test: vtgate_buffer.py: Move setup code from setUp to setUpModule.
  test: Add "demote_master_commands" to MysqlFlavor class.
  Include on clauses in impossible queries, to avoid only_full_group_by in aggregations with joins (vitessio#2433)
  Adds support for parsing queries with boolean value comparisons, i.e. true/false and =, !=, >, <, >=, <= (vitessio#2432)
  V3 Engine Code Refactor (vitessio#2423)
  Refactor impossible query generation, and support group by - (vitessio#2409)
  docker/test/run: Avoid using non-universal --tmpdir flag on mktemp (vitessio#2429)
  ...
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.

3 participants