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

Fixed #28574 -- Add Queryset.explain() method #9053

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
8 participants
@orf
Copy link
Contributor

orf commented Sep 10, 2017

Ticket

>> qs = Tag.objects.filter(name='test').all()
>> print(qs.explain())
Seq Scan on queries_tag  (cost=0.00..23.00 rows=5 width=50)
  Filter: ((name)::text = 'test'::text)
>> print(qs.explain(verbose=True))
Seq Scan on public.queries_tag  (cost=0.00..23.00 rows=5 width=50) (actual time=0.054..0.054 rows=0 loops=1)
  Output: id, name, parent_id, category_id
  Filter: ((queries_tag.name)::text = 'test'::text)
  Rows Removed by Filter: 5
Planning time: 0.382 ms
Execution time: 0.189 ms
>> print(qs.explain(verbose=True, format='json'))
[{'Execution Time': 0.177, 'Triggers': [], 'Planning Time': 0.419, 'Plan': {'Actual Loops': 1, 'Filter': "((queries_tag.name)::text = 'test'::text)", 'Node Type': 'Seq Scan', 'Rows Removed by Filter': 5, 'Alias': 'queries_tag', 'Output': ['id', 'name', 'parent_id', 'category_id'], 'Actual Startup Time': 0.067, 'Actual Total Time': 0.067, 'Relation Name': 'queries_tag', 'Parallel Aware': False, 'Plan Rows': 5, 'Plan Width': 50, 'Actual Rows': 0, 'Total Cost': 23.0, 'Schema': 'public', 'Startup Cost': 0.0}}]

I need to add docs, but I hope the general implementation is OK with regards to modifying the operations and features db-backend files, and their interaction with the compiler/queryset/query etc.

@orf orf force-pushed the orf:28574-queryset-explain branch 4 times, most recently from ca66689 to 9320d84 Sep 10, 2017

@@ -25,6 +25,7 @@ class DatabaseOperations(BaseDatabaseOperations):
'PositiveSmallIntegerField': 'unsigned integer',
}
cast_char_field_without_max_length = 'char'
explain_prefix = 'EXPLAIN'

This comment has been minimized.

@jschneier

jschneier Sep 10, 2017

Contributor

Need a trailing space here I think.

This comment has been minimized.

@orf

orf Sep 10, 2017

Contributor

I don't think so, it's used in the as_sql function which is a list of tokens, joined with a space

@orf orf force-pushed the orf:28574-queryset-explain branch from a67ac2c to 4cd5050 Sep 10, 2017

@orf orf changed the title Fixes #28574 -- Add Queryset.explain() method Fixed #28574 -- Add Queryset.explain() method Sep 10, 2017

for verbose, format in itertools.product((True, False), all_formats):
with self.subTest(verbose=verbose, format=format):
r = qs.explain(format=format, verbose=verbose)
self.assertTrue(isinstance(r, str))

This comment has been minimized.

@atombrella

atombrella Sep 11, 2017

Contributor

Use self.assertIsInstance instead.

This comment has been minimized.

@orf

orf Sep 11, 2017

Contributor

Done, thanks!

@orf orf force-pushed the orf:28574-queryset-explain branch 4 times, most recently from a6515be to f96c06b Sep 11, 2017

@orf orf force-pushed the orf:28574-queryset-explain branch 2 times, most recently from 68eaf7f to b1a7f8d Sep 19, 2017

@orf orf force-pushed the orf:28574-queryset-explain branch 3 times, most recently from 19c4b3a to 93681bc Sep 28, 2017


def test_explain_unknown_format(self):
qs = Tag.objects.filter(name='test').all()
with self.assertRaises(ValueError):

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

Please test the value of the message. Since it's variable, depending on the backend, I think it's fine to use supported_explain_formats with a check if it has a value.

This comment has been minimized.

@orf

orf Oct 6, 2017

Contributor

Can you elaborate? I'm not sure what you mean, how would I use supported_explain_formats in this context?

Tag.objects.filter(name='test').annotate(Count('children')),
Tag.objects.filter(name='test').values_list('name'),
Tag.objects.order_by().union(Tag.objects.order_by().filter(name='test')),
Tag.objects.all().select_for_update().filter(name='test')

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

Please add a trailing comma.

queries. The output is different for each database and version, do
not attempt to parse the output.


This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

Delete the extra space.

# tuples with integers and strings. Flatten them out into strings.
for row in result[0]:
if not isinstance(row, str):
yield " ".join(str(c) for c in row)

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

Single-quotes. Please check other places. Maybe you could also merge the checks to a single line?

@@ -46,6 +46,9 @@ class BaseDatabaseOperations:
UNBOUNDED_FOLLOWING = 'UNBOUNDED ' + FOLLOWING
CURRENT_ROW = 'CURRENT ROW'

# Prefix for EXPLAIN queries. None if the backend does not support this.
explain_prefix = None

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

Blank string instead?


def explain_query_prefix(self, output_format=None, verbose=False):
if not self.explain_prefix:
raise NotImplementedError("This backend does not support explaining query execution")

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

NotSupportedError, and please use single-quotes, and add a dot at the end.

@@ -48,6 +48,13 @@ class DatabaseFeatures(BaseDatabaseFeatures):
"""

@cached_property
def supported_explain_formats(self):
if self.connection.mysql_version >= (5, 6):

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

You can skip this check, as MySQL 5.5 is not supported anymore, and maybe it's better to move into a class-variable.

@@ -51,6 +51,8 @@ class DatabaseFeatures(BaseDatabaseFeatures):
supports_over_clause = True
supports_aggregate_filter_clause = True

supported_explain_formats = {'XML', 'JSON', 'YAML'}

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

Although it's default, TEXT is also a supported format.


.. warning::

This method is only useful as an aide for debugging poorly performing

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

aide -> aid ? I'm not a native speaker, but isn't there a difference between these two?

This comment has been minimized.

@orf

orf Oct 6, 2017

Contributor

It is correct as is, but I could reword it to is only useful to aid debugging if that reads better for non-native speakers?

This comment has been minimized.

@atombrella

atombrella Oct 7, 2017

Contributor

I hadn't seen the spelling "aide" before. I'm indifferent to both suggestions.


``explain()``

.. method:: explain(verbose=False, format=None)

This comment has been minimized.

@atombrella

atombrella Oct 6, 2017

Contributor

In PostgreSQL, there's also COST and BUFFERS. I didn't check Oracle or MySQL/MariaDB/SQLite documentation, but maybe **extra could deal with additional backend specific features? I'm suggesting it here, as a small note in case of no changes in the documentation will address it with a note.

Please add a .. versionadded:: 2.1.

@orf

This comment has been minimized.

Copy link
Contributor

orf commented Oct 7, 2017

Thanks for the review @atombrella, I've made the requested changes 👍

I wasn't sure about adding TEXT to the list of supported formats on postgres, as mysql doesn't have some kind of default format you can specify in the same was as postgres. But I guess it's fine for postgres.

@orf orf force-pushed the orf:28574-queryset-explain branch 2 times, most recently from db3ed35 to c3a65f4 Oct 7, 2017

@atombrella
Copy link
Contributor

atombrella left a comment

A few pedantic comments.

result = ['SELECT']
result = []
if self.query.explain_query:
result.append(self.connection.ops.explain_query_prefix(self.query.explain_format,

This comment has been minimized.

@atombrella

atombrella Oct 28, 2017

Contributor

No hanging indent.

This comment has been minimized.

@orf

orf Nov 27, 2017

Contributor

Fixed 👍

self.assertIn('does not exist is not a recognised format', str(exc.exception))

if connection.features.supported_explain_formats:
self.assertIn(', '.join(connection.features.supported_explain_formats), str(exc.exception))

This comment has been minimized.

@atombrella

atombrella Oct 28, 2017

Contributor

Just for completeness, shouldn't Allowed formats: also be in this assertion?

@@ -51,6 +51,8 @@ class DatabaseFeatures(BaseDatabaseFeatures):
supports_over_clause = True
supports_aggregate_filter_clause = True

supported_explain_formats = {'TEXT', 'XML', 'JSON', 'YAML'}

This comment has been minimized.

@atombrella

atombrella Oct 28, 2017

Contributor

I think the style is not to have a blank line between the feature-flags, except in the base-backend. Please also see the MySQL feature flag.

@orf orf force-pushed the orf:28574-queryset-explain branch 2 times, most recently from e4c62fe to 173a16b Oct 29, 2017

@pope1ni
Copy link
Contributor

pope1ni left a comment

Thanks for tackling this - it'll be useful not having to dredge up the query and drop out to a database shell.

I have made many comments regarding the various options that can be provided and I think that the documentation would benefit from highlighting some of the backend-specific quirks.

@@ -46,6 +46,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
SET V_I = P_I;
END;
"""
supported_explain_formats = {'JSON'}

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Perhaps worth adding a comment to explain the absence of TEXT with respect to #9053 (comment).

Even better, TEXT could be added and translated to TRADITIONAL in explain_query_prefix().
(See the last bullet point in the documentation.)

This comment has been minimized.

@orf

orf Nov 11, 2017

Contributor

I've added TRADITIONAL to the supported formats, and translated text to TRADITIONAL in the explain_query_prefix function

if output_format:
supported_formats = self.connection.features.supported_explain_formats
if output_format.upper() not in supported_formats:
msg = '{0} is not a recognised format'.format(output_format)

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Make output_format uppercase in the exception message so that it matches those in supported_formats?

This comment has been minimized.

@orf

orf Nov 11, 2017

Contributor

Done 👍

supported_formats = self.connection.features.supported_explain_formats
if output_format.upper() not in supported_formats:
msg = '{0} is not a recognised format'.format(output_format)
if supported_formats:

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Is this condition necessary? Surely all backends will support a default TEXT format?
(Even if it can't be explicitly provided in the query...)

This comment has been minimized.

@orf

orf Nov 11, 2017

Contributor

I'm not sure. In mysql's case TEXT doesn't exist, it's some ridiculous table structure that is quite hard to reason about. I'd like to keep the default as unspecified if possible, if the database doesn't support any formats other than the default we should just error without any Allowed Formats: message IMO. Open to discussion though.

if output_format.upper() not in supported_formats:
msg = '{0} is not a recognised format'.format(output_format)
if supported_formats:
msg += '. Allowed formats: {0}'.format(', '.join(supported_formats))

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Move the leading period to the initial definition of msg.

This comment has been minimized.

@orf

orf Nov 11, 2017

Contributor

Done 👍

@@ -50,6 +50,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
$$ LANGUAGE plpgsql;"""
supports_over_clause = True
supports_aggregate_filter_clause = True
supported_explain_formats = {'TEXT', 'XML', 'JSON', 'YAML'}

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Sort these formats alphabetically.

This comment has been minimized.

@orf

orf Nov 11, 2017

Contributor

Done 👍

elif verbose:
# EXTENDED format is deprecated in 5.7 and above.
prefix += ' EXTENDED '
return prefix

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

EXTENDED is removed in MySQL 8.0 as it has become part of the default output as of 5.7. This needs to be changed to only be added for MySQL < 5.7

PARTITIONS is also missing but the same situation as EXTENDED above also applies. It is also mutually exclusive with EXTENDED and FORMAT. It is probably not worth adding the capability to get the PARTITIONS output only for 5.6

According to the documentation the JSON format already includes the extra information provided by EXTENDED and PARTITIONS.

I suggest changing this method to something like the following (which includes the text format option translation mentioned in another comment):

def explain_query_prefix(self, output_format=None, verbose=False):
    prefix = super().explain_query_prefix(output_format, verbose)
    if output_format:
        if output_format.upper() == 'TEXT':
            output_format = 'TRADITIONAL'
        prefix += ' FORMAT=%s ' % output_format
    if self.connection.mysql_version < (5, 7) and verbose and output_format is None:
        # EXTENDED and FORMAT are mutually exclusive options.
        # EXTENDED is deprecated (and not required) in 5.7 and removed in 8.0
        prefix += ' EXTENDED '
    return prefix

This makes it easier to remove the cruft when MySQL 5.6 is no longer supported.

I'm not sure whether it is worth having the default for verbose be None thus turning this flag tri-state. This would would allow for raising an exception when verbose is explicitly set to False for MySQL >= 5.7, but I'll defer to someone else's better judgement.

This comment has been minimized.

@orf

orf Nov 27, 2017

Contributor

Thank you for this, and for the code snippet. I'll work on adding support for this.

As I understand it mysql's explain output is very different from other databases, the default output is in the form of a single tuple, and the column names are very important in the output. Currently there isn't a way to return the column names from queries, which I think makes this a little bit useless on mysql currently. Would you agree, and would you know any workarounds? I'm a bit loathe to add support for returning column names from some of the internals just for this specific case.

This comment has been minimized.

@orf

orf Nov 27, 2017

Contributor

Ignore that, you can get the column names, I was just being dense 👍

This comment has been minimized.

@pope1ni

pope1ni Nov 27, 2017

Contributor

I think it is worth just spewing it out as a complete text blob. Otherwise we would be adding specialised parsing which is not worth it for a textual output.

This comment has been minimized.

@pope1ni

pope1ni Nov 27, 2017

Contributor

Also, could you add in the handling for EXTENDED as I had in my comment above? This will ensure that Django will output the same, regardless of the version of MySQL.

This comment has been minimized.

@orf

orf Nov 27, 2017

Contributor

Done 👍

def explain_query_prefix(self, output_format=None, verbose=False):
prefix = super().explain_query_prefix(output_format, verbose)

extra = {}

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Following on from #9053 (comment) which didn't seem to have a response...

PostgreSQL has the following additional options in all supported versions: BUFFERS, COSTS, TIMING.
Version 10+ also supports an additional SUMMARY option. (See documentation for more details.)

Note that some of these options are already enabled by default but can be explicitly disabled. It could be easier to just enable all of these options when verbose is set if it doesn't hugely increase the time taken to execute the statement.

This comment has been minimized.

@orf

orf Oct 30, 2017

Contributor

Oh, I must have forgotten to respond to that comment. I didn't want to go too deep into adding support for all the various flags, but I think that I will after reading your comments. I'll let .explain() accept arbitrary keyword arguments and the various explain_query_prefix functions can do what they want with them.

This comment has been minimized.

@pope1ni

pope1ni Oct 31, 2017

Contributor

Yeah. There are quite a few, and this is anything but standard across backends. The advantage is being able to access all of this information without needing to pop out to the database shell and restricting the flags may cripple the use of this feature for some.

Adding support via kwargs sounds sensible. The irritation may be determining the default state of the various flags based on the value of verbose. So it may be easier to keep the interface simple and just "enable all the things" if verbose is True, where they are not costly in terms of execution time, and then provide analyze separately in kwargs.

extra['FORMAT'] = output_format
if verbose:
extra['VERBOSE'] = 'true'
extra['ANALYZE'] = 'true'

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

I don't think that ANALYZE should be set when using verbose. This makes it execute the query which could take a long time, especially when users will think this means VERBOSE. Perhaps a separate parameter should be used for triggering this behaviour?

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

In addition, use of ANALYZE can lead to modification of data which should be documented - even with a SELECT query if a function is executed - see the documentation. It might make sense to highlight that a transaction should be used which can be aborted if the user wants to avoid this.

This comment has been minimized.

@orf

orf Oct 30, 2017

Contributor

I didn't consider this, it absolutely should be documented and not the default when verbose=true!

This comment has been minimized.

@pope1ni

pope1ni Oct 31, 2017

Contributor

Something like this comes to mind:

with transaction.atomic():
    transaction.set_rollback(True)
    print(queryset.explain(analyze=True))
self.query.explain_format,
self.query.explain_verbose)
result.append(prefix)
result.append('SELECT')

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Does this mean that support for this is limited to SELECT statements?

  • PostgreSQL supports SELECT, INSERT, UPDATE, DELETE, and others that are irrelevant to Django...
  • MySQL support SELECT, INSERT, UPDATE, DELETE, REPLACE.

This comment has been minimized.

@orf

orf Oct 30, 2017

Contributor

I wanted to enable this just for SELECT queries to begin with. To be honest I'm not sure how I could enable it for the others, .explain() would have to be added before an .update()/delete() call and would have to modify the returned values. If you have any ideas how this could be done I would love to hear them, but perhaps we can add this as a separate change.

This comment has been minimized.

@pope1ni

pope1ni Oct 31, 2017

Contributor

Agreed. A follow-up ticket to investigate the possibility of this would be a good idea, along with a discussion on the mailing list.

One way is to implement it as you have said, another could be some sort of context manager approach, e.g.

with queryset.explain() as output:
    updated = queryset.update(field=value)
    print(output)

In this case, updated ought to be 0. If queryset.explain(analyze=True) were used, updated would be the number of records modified (assuming the backend can report this at the same time).

This comment has been minimized.

@atombrella

atombrella Nov 1, 2017

Contributor

Adding hooks in as_sql in the different compilers to check whether the query should be executed or explained might be an option? I think it's necessary to do changes in those methods.

if self.query.explain_query:
prefix = self.connection.ops.explain_query_prefix(
self.query.explain_format,
self.query.explain_verbose)

This comment has been minimized.

@pope1ni

pope1ni Oct 30, 2017

Contributor

Hanging indent please, and it should be possible to remove the temporary prefix.

This comment has been minimized.

@orf

orf Nov 11, 2017

Contributor

Done 👍

@@ -154,6 +154,8 @@ Models
~~~~~~

* Models can now use ``__init_subclass__()`` from :pep:`487`.
* The new :meth:`.QuerySet.explain` method allows displaying the execution

This comment has been minimized.

@atombrella

atombrella Nov 1, 2017

Contributor

Please add a blank line between the *.

This comment has been minimized.

@orf

orf Nov 11, 2017

Contributor

Done 👍

@orf orf force-pushed the orf:28574-queryset-explain branch from 173a16b to f4c80cc Nov 11, 2017

@orf orf force-pushed the orf:28574-queryset-explain branch 3 times, most recently from 1b41e90 to c2f69b9 Nov 27, 2017

The ``format`` parameter can output the explanation in different formats,
depending on the database in use. If this parameter is not passed then the
default database format is used, typically text-based. Currently PostgreSQL
supports TEXT, JSON, YAML and XML, while MySQL supports TEXT and JSON.

This comment has been minimized.

@adamchainz

adamchainz Apr 1, 2018

Member

'and' instead of 'while'? Using 'while' implies MySQL is inferior 😱

This comment has been minimized.

@orf

orf Apr 2, 2018

Contributor

:D good point, we would not want people to think that.

if supported_formats:
msg += ' Allowed formats: {0}'.format(', '.join(supported_formats))
raise ValueError(msg)

This comment has been minimized.

@adamchainz

adamchainz Apr 1, 2018

Member

What about erroring if options is non-empty at this point? Subclasses should have already consumed their arguments from it, and if there's anything left it's probably a mistake, like explain(formatt='json')

This comment has been minimized.

@orf

orf Apr 2, 2018

Contributor

I'm a bit on the fence about this, on one hand it would be nice for this to be pretty flexible with what it accepts as I think it's primarily going to be used as a debugging helper in a REPL, but then again if you're passing in options to any backend except Postgres then something is going wrong.

In any case I've made it throw a RuntimeError, let me know what you think

This comment has been minimized.

@adamchainz

adamchainz Apr 2, 2018

Member

Best to be conservative and error rather than silently swallow mistyped arguments :) I think ValueError or TypeError are more appropriate, they're normally used for argument validation.

@@ -258,3 +259,21 @@ def window_frame_range_start_end(self, start=None, end=None):
'and FOLLOWING.'
)
return start_, end_

def explain_query_prefix(self, format=None, **options):
prefix = super().explain_query_prefix(format, **options)

This comment has been minimized.

@adamchainz

adamchainz Apr 1, 2018

Member

no need to pass options up since they are all being consumed here

@orf orf force-pushed the orf:28574-queryset-explain branch from 7389117 to 5c167b8 Apr 2, 2018

@orf

This comment has been minimized.

Copy link
Contributor

orf commented Apr 2, 2018

Thanks for the review, I've made the changes requested. The docs are failing to build but it appears to be an issue in the 'writing-documentation' file of all places, which I've not touched.

@orf

This comment has been minimized.

Copy link
Contributor

orf commented Apr 2, 2018

buildbot, test on oracle.

self.assertTrue(result)

@skipUnlessDBFeature('supports_explaining_query_execution')
@unittest.skipIf(connection.vendor == 'postgresql', "PostgreSQL specific")

This comment has been minimized.

@adamchainz

adamchainz Apr 2, 2018

Member

Surely 'All backends except PostgreSQL' ? :)

if supported_formats:
msg += ' Allowed formats: {0}'.format(', '.join(supported_formats))
raise ValueError(msg)

This comment has been minimized.

@adamchainz

adamchainz Apr 2, 2018

Member

Best to be conservative and error rather than silently swallow mistyped arguments :) I think ValueError or TypeError are more appropriate, they're normally used for argument validation.

@django django deleted a comment from orf Apr 4, 2018

def test_explain(self):
querysets = [
Tag.objects.filter(name='test').all(),
Tag.objects.filter(name='test').select_related("parent"),

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

Use single quotes consistently.

Seq Scan on blog (cost=0.00..35.50 rows=10 width=12)
Filter: (title = 'My Blog'::bpchar)

This method is currently supported by all built in database backends with the

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

chop "currently". Is there any plan to add Oracle support? I didn't follow your discussion closely but I thought you basically determined it's infeasible.

default database format is used, typically text-based. Currently PostgreSQL
supports TEXT, JSON, YAML and XML, and MySQL supports TEXT and JSON.

Some databases, e.g PostgreSQL, accept flags that can return more

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

I think you can chop "e.g PostgreSQL" considering it's mentioned later.


.. warning::

This method is only useful to aid debugging poorly performing

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

I don't think this should be in a warning box. Put it somewhere in the main text above.

@@ -2476,6 +2476,55 @@ Class method that returns an instance of :class:`~django.db.models.Manager`
with a copy of the ``QuerySet``’s methods. See
:ref:`create-manager-with-queryset-methods` for more details.


This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

chop blank line

@unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific")
def test_postgres_explain_options(self):
qs = Tag.objects.filter(name='test').all()

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

remove some blank lines


@unittest.skipUnless(connection.vendor == 'postgresql', "PostgreSQL specific")
def test_postgres_explain_options(self):
qs = Tag.objects.filter(name='test').all()

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

is .all() required?

@@ -83,6 +85,99 @@ def setUpTestData(cls):
Cover.objects.create(title="first", item=i4)
Cover.objects.create(title="second", item=cls.i2)

@skipIfDBFeature('supports_explaining_query_execution')

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

Could you please put these in a new test_explain.py file? This file is already a bit too long and unorganized.


.. warning::

On some supported databases these flags can cause the query to be executed

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

I'm not sure what "supported databases" means. Is "supported" adding some meaning?

@@ -48,6 +48,8 @@ class DatabaseFeatures(BaseDatabaseFeatures):
SET V_I = P_I;
END;
"""
# MySQL has TRADITIONAL instead of TEXT, but we alias TRADITIONAL to TEXT for consistency

This comment has been minimized.

@timgraham

timgraham Apr 4, 2018

Member

Chop "we" per our style guide.

@orf orf force-pushed the orf:28574-queryset-explain branch 3 times, most recently from efd2960 to 20ad0c7 Apr 5, 2018

@orf

This comment has been minimized.

Copy link
Contributor

orf commented Apr 5, 2018

Thank you for the review @timgraham, I've made the changes you've requested - they all made sense and I didn't want to reply to each and spam you.

After splitting the tests out I found that they failed with the union queryset, it was actually doing entirely the wrong thing (executing the raw query without the explain prefix). I've fixed this and added a test case for diff as well, as well as actually testing that the explain prefix is in the executed SQL.

There is a missing space in the docs, I'll push a fix for that after the Oracle test suite passes.

@django django deleted a comment from orf Apr 5, 2018

@django django deleted a comment from orf Apr 5, 2018

@orf orf force-pushed the orf:28574-queryset-explain branch 2 times, most recently from c2c0458 to 475425f Apr 13, 2018

@timgraham timgraham force-pushed the orf:28574-queryset-explain branch 2 times, most recently from f2b3329 to 5e43070 Apr 17, 2018

@timgraham
Copy link
Member

timgraham left a comment

Silence MySQL warnings in tests? e.g.

django/db/backends/mysql/base.py:71: Warning: (1003, "(/* select#1 */ select `test_django`.`queries_tag`.`id` AS `id`,`test_django`.`queries_tag`.`name` AS `name`,`test_django`.`queries_tag`.`parent_id` AS `parent_id`,`test_django`.`queries_tag`.`category_id` AS `category_id` from `test_django`.`queries_tag`) union (/* select#2 */ select `test_django`.`queries_tag`.`id` AS `id`,`test_django`.`queries_tag`.`name` AS `name`,`test_django`.`queries_tag`.`parent_id` AS `parent_id`,`test_django`.`queries_tag`.`category_id` AS `category_id` from `test_django`.`queries_tag` where (`test_django`.`queries_tag`.`name` = 'test'))")
  return self.cursor.execute(query, args)
self.assertIsInstance(result, str)
self.assertTrue(result)

@unittest.skipIf(connection.vendor == 'postgresql', 'Postgresql gives unrecognized EXPLAIN option "test"')

This comment has been minimized.

@timgraham

timgraham Apr 17, 2018

Member

3rd party backends might not want this test either. Add a database feature like validates_explain_options?

@orf orf force-pushed the orf:28574-queryset-explain branch from 5e43070 to 16abbca Apr 18, 2018

@timgraham

This comment has been minimized.

Copy link
Member

timgraham commented Apr 19, 2018

I found that something like this will silence the warnings: warnings.filterwarnings('ignore', '\(1003, *', category=MySQLdb.Warning). Not sure if we want to add that in runtests.py or somewhere else. category=MySQLdb.Warning could also be omitted to avoid a try/except ImportError for if MySQL isn't installed.

@orf

This comment has been minimized.

Copy link
Contributor

orf commented Apr 19, 2018

buildbot, test on oracle.

@orf

This comment has been minimized.

Copy link
Contributor

orf commented Apr 19, 2018

Thanks @timgraham. I've added the warning filter to runtests.py, only if MySQLdb is installed. I figured that the warning would only be useful with MySQL tests and we wouldn't want to inadvertently filter out other warnings.

# Ignore MySQL informational warnings that are triggered with queryset.explain()
warnings.filterwarnings('ignore', '\(1003, *', category=MySQLdb.Warning)
except ImportError:
pass

This comment has been minimized.

@pope1ni

pope1ni Apr 19, 2018

Contributor

This would be better styled as:

try:
    import MySQLdb
except ImportError:
    pass
else:
    # ...
    warnings.filterwarnings(...)

@timgraham timgraham force-pushed the orf:28574-queryset-explain branch 2 times, most recently from 24e3efe to 460423f Apr 19, 2018

@timgraham timgraham force-pushed the orf:28574-queryset-explain branch from 460423f to c1c163b Apr 19, 2018

@timgraham timgraham merged commit c1c163b into django:master Apr 19, 2018

17 checks passed

docs Build #22219 ended
Details
flake8 Build #22378 ended
Details
isort Build #22382 succeeded in 20 sec
Details
pull-requests-javascript Build #18752 ended
Details
pull-requests-trusty/database=mysql,label=trusty-pr,python=python3.5 Build #17896 ended
Details
pull-requests-trusty/database=mysql,label=trusty-pr,python=python3.6 Build #17896 ended
Details
pull-requests-trusty/database=mysql_gis,label=trusty-pr,python=python3.5 Build #17896 ended
Details
pull-requests-trusty/database=mysql_gis,label=trusty-pr,python=python3.6 Build #17896 ended
Details
pull-requests-trusty/database=postgis,label=trusty-pr,python=python3.5 Build #17896 ended
Details
pull-requests-trusty/database=postgis,label=trusty-pr,python=python3.6 Build #17896 ended
Details
pull-requests-trusty/database=postgres,label=trusty-pr,python=python3.5 Build #17896 ended
Details
pull-requests-trusty/database=postgres,label=trusty-pr,python=python3.6 Build #17896 ended
Details
pull-requests-trusty/database=spatialite,label=trusty-pr,python=python3.5 Build #17896 ended
Details
pull-requests-trusty/database=spatialite,label=trusty-pr,python=python3.6 Build #17896 ended
Details
pull-requests-trusty/database=sqlite3,label=trusty-pr,python=python3.5 Build #17896 ended
Details
pull-requests-trusty/database=sqlite3,label=trusty-pr,python=python3.6 Build #17896 ended
Details
pull-requests-windows/database=sqlite3,label=windows,python=Python36 Build #14205 ended
Details

@orf orf deleted the orf:28574-queryset-explain branch Apr 19, 2018

@orf

This comment has been minimized.

Copy link
Contributor

orf commented Apr 19, 2018

Thanks everyone who helped with this, and thanks for the reviewing @timgraham 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment