Skip to content
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

tickets/DM-16821 #107

Merged
merged 1 commit into from Dec 17, 2018
Merged

tickets/DM-16821 #107

merged 1 commit into from Dec 17, 2018

Conversation

natelust
Copy link
Contributor

Format sql query debug message

queryParams = compiledQuery.params
for name, value in queryParams.items():
queryString = queryString.replace(":"+name, str(value))
_LOG.debug("full query: %s", queryString)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be some variant on that that works, but that compile_kwargs was producing something, that was not a valid sqllite query because of the True, that compiles to true, and sqlite only wants it to be "1" or 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want it in exactly the same format as can be executed against database? Why? I doubt that you approach can produce actual executable statement (especially for other backends besides sqlite), just imagine that you have :param1 and :param10 in your compiled statement.

You can also try pass engine object as the first argument to compile() (together with compile_kwargs) but I doubt that it will improve things as rendering with literal_binds happens in sqlalchemy itself, no in engine.

Copy link
Member

Choose a reason for hiding this comment

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

The use case here was trying to debug why the query was returning no results, for which it actually was useful to paste the query into a SQLite terminal and fiddle with it (i.e. running subqueries individually, removing various restrictions) to understand what was going wrong.

I think I'm gathering that we may not want to change the logging to always do this insertion, but we should think about other ways to do that kind of debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Andy I take your point as a good one, but like Jim said, it would be nice to be able to copy and paste into databases to debug. I see this working with either:

  1. switching to a more robust regular expression for substitution to avoid issues like you highlight or,
  2. Simply also print out the dictionary of parameter strings, which would not be copy pastable, but perhaps would be enough for an interested party to do their own formatting prior to database execution,

Copy link
Contributor

Choose a reason for hiding this comment

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

Dumping query with parameters dictionary is probably easiest to implement. I agree that we do need better diagnostics for users to understand what happens in preflight (e.g. why returned QGraph is empty). Messing with SQL might work for expert users but for others something more friendly should be implemented. Probably adding an extra option for preflight to exercise different sub-queries and to dump their statistics might be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the latest push and comments on Jira, I was able to resolve this in a way that should make everyone happy.

@natelust natelust force-pushed the tickets/DM-16821 branch 2 times, most recently from 753a3d2 to 19a90b2 Compare December 16, 2018 17:46
@natelust natelust merged commit a7acee6 into master Dec 17, 2018
@timj timj deleted the tickets/DM-16821 branch April 22, 2020 22:02
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.

None yet

3 participants