Skip to content

Conversation

jf205
Copy link
Contributor

@jf205 jf205 commented Jan 3, 2020

The topics that currently live under Advanced QL in the 'Learning CodeQL' Sphinx project are a bit random. I've started to move them around into (hopefully) more logical/helpful locations:

@max-schaefer would you mind reviewing the technical aspect of these changes please?

@shati-patel do you agree with this restructuring? (and please can you review the text?)

TODO

@jf205 jf205 requested a review from shati-patel as a code owner January 3, 2020 12:14
@jf205 jf205 requested a review from max-schaefer January 3, 2020 12:14
max-schaefer
max-schaefer previously approved these changes Jan 3, 2020
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 is a sensible restructuring! Just a few minor comments, otherwise LGTM.


Going from ``Element -> File`` and ``Element -> Location -> StartLine`` is linear--that is, there is only one ``File``, ``Location``, etc. for each ``Element``.

However, as written it is difficult for the optimizer to pick out the best ordering. Generally, we want to do the quick, linear parts first, and then join on the resultant larger tables. Joining first and then doing the linear lookups later would likely result in poor performance. We can initiate this kind of ordering by splitting the above predicate as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

... would likely result in poor performance. We can initiate this kind of ordering ...

Sounds like we want to initiate the "bad" ordering! Perhaps swap the order of the two preceding sentences.

.. code-block:: ql

predicate similar(Element e1, Element e2) {
e1.getName() = e2.getName() and
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 half of this topic uses the "autoformatted" style, in particular 2 spaces for indentation instead of 4. Probably worth doing that here too.

(I wouldn't worry about it in the older standalone topics, but the difference is quite noticeable here.)

@jf205
Copy link
Contributor Author

jf205 commented Jan 3, 2020

Thanks @shati-patel. I've addressed both of your comments.

shati-patel
shati-patel previously approved these changes Jan 3, 2020
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 👍

Another mini suggestion, but I'm happy to merge as-is too.

@jf205 jf205 dismissed stale reviews from shati-patel via 47f61f3 January 3, 2020 15:55
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.

👍

@shati-patel shati-patel merged commit 9b9d712 into github:master Jan 3, 2020
@jf205 jf205 deleted the advanced-ql branch January 8, 2020 12:07
@jf205
Copy link
Contributor Author

jf205 commented Feb 4, 2020

I have now (rather belatedly) added redirects for the topics that were removed in this PR.

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.

3 participants