-
Notifications
You must be signed in to change notification settings - Fork 312
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
Support explain plan for the TDS adapter #273
Conversation
params = prepare_params(params) | ||
|
||
case Tds.query_multi(conn, build_explain_query(query), params, opts) do | ||
{:ok, [_, %Tds.Result{} = result, _]} -> |
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.
Not sure if the order is deterministic, but looks like it's (still need to confirm on TDS code).
/edit After digging and testing, looks like that's safe.
lib/ecto/adapters/tds/connection.ex
Outdated
|> IO.iodata_to_binary() | ||
end | ||
|
||
defp format_result_as_table(columns, rows) do |
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.
Those function are exactly the same as used on myxql adapter, should I move those to Ecto.Adapters.SQL
and put a @doc 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.
I think we can introduce a function called format_table
in there and make it generally available. :)
Looks great, awesome job! ❤️ |
@josevalim that's how a TDS explain looks like:
That will look ugly when printed to a console that wraps the lines, but you can copy&paste to an editor, save to a file, or configure the console if that's possible. Anyway, I suppose that's good enough for the first version. Wdyt? |
Awesome job! ❤️ |
That's still a draft but it's working so you can test it locally. I'm not a SQL Server expert but looks like the result set has all the information we would need for a v1 (like the screenshot on the link below), so no need to parse XML.I believe it's good enough for review, see #273 (comment).
Also see #231 (comment) for reference.
Wdyt?