Skip to content

docs: make a start on query debugging topic #1914

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

Merged
merged 6 commits into from
Dec 11, 2019

Conversation

jf205
Copy link
Contributor

@jf205 jf205 commented Sep 10, 2019

This PR adds an incomplete draft of a topic currently named 'Query writing: common issues'.
I was prompted to start writing this following various discussions over recent weeks.
At the moment it contains a pretty sparse list of QL constructs to avoid when writing queries.
I would very much welcome any suggestions about what else to include.

@jf205
Copy link
Contributor Author

jf205 commented Sep 23, 2019

@max: before you go on leave would you mind having a quick look at this topic on debugging QL performance please?

Can you think of anything that should be added? (bearing in mind that it is intended to be an introductory topic).

@felicitymay
Copy link
Contributor

It would be really good if it were possible to review this so that it can be published in early November.

@lcartey - do you have any more feedback or suggestions for other reviewers who might be able to help with other issues to watch out for?

@jf205 jf205 changed the base branch from master to rc/1.23 December 11, 2019 09:26
@jf205 jf205 marked this pull request as ready for review December 11, 2019 10:47
@jf205 jf205 requested a review from shati-patel as a code owner December 11, 2019 10:47
@jf205
Copy link
Contributor Author

jf205 commented Dec 11, 2019

This has been sitting around for a while, but I think this is worth getting into the rc/1.23 branch. We can always expand it later (fyi @shati-patel).

@max-schaefer, would you mind sanity checking and merging for me please?

@jf205 jf205 requested a review from lcartey December 11, 2019 11:17
Copy link
Contributor

@max-schaefer max-schaefer left a comment

Choose a reason for hiding this comment

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

I agree that this would be very useful to have! I have a number of suggestions.

Comment on lines 25 to 30
For instance, in the following case none of the parameters are related in the example predicate ``methodAndAClass``, and therefore the results are unrestricted::

// BAD! Cross-product
predicate methodAndAClass(Method m, Class c) {
any()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This example is a bit too contrived. How about this?


For instance, consider the following predicate that checks whether a Java method m may access a field f:

predicate mayAccess(Method m, Field f) {
  f.getAnAccess().getEnclosingCallable() = m
  or
  not exists(m.getBody())
}

The predicate holds if m contains an access to f, but also conservatively assumes that methods without bodies (for example native methods) may access any field at all.

What this means, however, is that if m is a native method, then the table computed by mayAccess contains a row m, f for all fields f in the code base, resulting in a potentially very large table.

Here, the class ``Method getToString()`` is equivalent to ``predicate getToString(Class this, Method result)``, in which the parameters are unrestricted.
To avoid making this mistake, ``this`` should be restricted in the member predicate ``getToString()`` on the class ``Foo``.

Finally, consider a predicate of the following form::
Copy link
Contributor

Choose a reason for hiding this comment

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

The first example above now has this form, so perhaps this paragraph can be shortened.

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>
@jf205
Copy link
Contributor Author

jf205 commented Dec 11, 2019

Thanks for the suggestions @max-schaefer! I think they have all been addressed now.

Copy link
Contributor

@max-schaefer max-schaefer left a comment

Choose a reason for hiding this comment

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

Great! Two more minor suggestions.

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>
max-schaefer
max-schaefer previously approved these changes Dec 11, 2019
@jf205
Copy link
Contributor Author

jf205 commented Dec 11, 2019

Thanks @max-schaefer.

@lcartey requested two minor changes on an earlier iteration of this topic. I've made those changes, but we'll need to wait for approval but it can be merged.

@shati-patel
Copy link
Contributor

I'd like to have a quick look too - will review/merge after lunch!

Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks, this will be a useful topic to have!

I've made a few suggestions, but feel free to ignore them if you think they're unnecessary (apart from the typo).

Co-Authored-By: shati-patel <42641846+shati-patel@users.noreply.github.com>
@jf205
Copy link
Contributor Author

jf205 commented Dec 11, 2019

Thanks @shati-patel 👀

@shati-patel shati-patel dismissed lcartey’s stale review December 11, 2019 14:23

comments addressed

Co-Authored-By: shati-patel <42641846+shati-patel@users.noreply.github.com>
Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thank you!

@shati-patel shati-patel merged commit f2d1e53 into github:rc/1.23 Dec 11, 2019
@jf205 jf205 deleted the query-debugging branch January 8, 2020 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants