-
Notifications
You must be signed in to change notification settings - Fork 25
Make QuerySet.explain() return parsable JSON #340
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, less pprint
too!
To me, it's not an improvement in readability. Before:
After:
|
django_mongodb_backend/compiler.py
Outdated
result.append(f"{key}: {formatted_value}") | ||
return result | ||
# explain() expects a list and joins on a newline. Concatenate no lines | ||
return [json_util.dumps(explain)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use
return [json_util.dumps(explain)] | |
return [json_util.dumps(explain, indent=4, ensure_ascii=False)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since json_util.dumps()
is a pymongo specific json parsing function, I don't think we'll need the ensure_ascii=False
override.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about ensure_ascii
, but I'm wondering if you expect some difference in the output by using json_util.dumps()
instead of json.dumps()
?
We didn't have tests in this repo when I originally implemented this, but it would be useful to now have at least one test of the output in tests/queries_/test_explain.py
(new file).
I'm fine with ident=4
but just to be clear, that introduces the same "nightmare to parse" issue of newlines. Basically, your original usage mistake was not using print(Model.objects.explain())
so the newlines weren't rendered nicely. I think it's fine to make this change anyway. Incidentally, is there a use case for calling json.loads()
on the result of explain()
or was that just an attempt at making the output more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about ensure_ascii, but I'm wondering if you expect some difference in the output by using json_util.dumps() instead of json.dumps()?
json_util
properly handles the case of non-json-serializable BSON types (I.e. ObjectId()
). We know we're getting a dictionary from a mongodb query and we want it to be parseable. This allows everything to be viewed, and if it ever fails, that's a bug against PyMongo rather than this library.
We didn't have tests in this repo when I originally implemented this, but it would be useful to now have at least one test of the output in tests/queries_/test_explain.py (new file).
Sure. I can add that.
I'm fine with ident=4 but just to be clear, that introduces the same "nightmare to parse" issue of newlines. ...
It actually doesn't. It does keep the \n
ticks in pprint
, but in a much more readable way.
For me it's also a QOL issue. For exceptionally large queries, it gets nauseating to read an entire rendered print output so my standard workflow is adding it to a dictionary and then iterate through the query each piece. So in this new world all three paths become viable:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensure_ascii option was to handling some letters, like Spanish letter ñ, or ó, and so on. If this flag was in true, those letter got broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json dump can be prettified
Ah, to @WaVEV 's point, that should be mitigated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the only thing: what if we have a character like ñ
6706112
to
abc335f
Compare
Co-authored-by: Tim Graham <[email protected]>
abc335f
to
ced5649
Compare
Context
Calling
explain()
is extremely useful in debugging a MongoDB query. However, parsing the output of theexplain
call is a nightmare. This is because we take each line and format them using pprint to accommodate Django's nativeexplain
functionality, which joins all information line by line.Solution
Rather than split key/values into multiple lines, we should just dump the json as one string blob in the list. This way
.explain
can easily leveragejson.load
orjson_util.load
.Changes in this PR
json_util
json_util.dumps(..., indent=4)
and return that in a list of length 1.Change
Before
After