11.11.2015 01:25, liviusliv...@poczta.onet.pl wrote:
>
> Is this pull request useful at all or this is useless for FB team?
> I try to start to teach FB code, to work in the future on more useful and
> harder topics.
> But i must get start point, will be good to see feedback (and also
> criticism)
Generally, providing some structural information in the machine-readable
form is a good idea. Parsing plain text is terrible, especially when
this text changes from version to version.
When I was implementing the explained plan output, the original idea was
to have it represented in both binary form (some kind of byte code) and
textual one. The former one could be used by external tools for
automated processing while the latter one is the user-friendly output
for API and ISQL. This way, alternative format generators could be
layered similarly to blob filters, even if implemented differently (e.g.
as built-in functions).
But I never did the binary format. Why? Because nowadays nobody wants to
parse yet another proprietary format just to get the data. There are
de-facto standards like XML and JSON for that purposes, with existing
parsers and the whole infrastructure.
However, choosing XML over JSON or vice versa is IMHO a way nowhere.
Whatever one we choose, half of users will be against the chosen format
and ask us to support the second one. So we should either support both
out of the box or somehow stack custom format generators as plug-ins.
Just to summarize: I do support your work on the XML plan
representation, but we need to think carefully how it's going to be done.
Now back to your patch. The coding style issue was always mentioned,
please respect the existing style.
I don't like extending the print() method with type-of-format argument.
It wasn't really good even for legacy vs explained output and becomes
even worse for XML and other formats. Initially I was going to suggest
separate methods instead: printPlan(), explainAsText(), explainAsXml()
etc. But now I think that custom builder classes would be a better
approach - they would allow to separate data access methods from the
printing stuff and simplify pluggability, if required. I will try to
refactor the existing printing code into classes, so that you could
follow this approach for your implementation.
OPT_get_plan() may remain "as is" (with format-type specifier argument),
but I'd rather use enum instead of int.
As for the API usage, a better approach would be to use a two-byte tag
sequence, i.e.:
{ isc_info_sql_explain_plan, isc_info_sql_explain_type_xxx }
with:
isc_info_sql_explain_type_text 1
isc_info_sql_explain_type_xml 2
isc_info_sql_explain_type_json 3
...
rather than wasting the primary tag space with
isc_info_sql_explain_plan_xml, isc_info_sql_explain_plan_json, etc.
Finally, I'm not sure we need additional output formats in the Trace API.
As for the XML output itself -- tags / attributes / etc -- I don't have
something useful to comment as I almost never use XML myself ;-) So I
invite others to continue discussing this part of the patch.
Dmitry
------------------------------------------------------------------------------
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
