Default to "Full texts" when running ANALYZE

[nunoperalta]

Hey

When the query begins with ANALYZE (MariaDB) or EXPLAIN ANALYZE (MySQL),
could you please default to "Full texts", so we don't have to run the query again?

ANALYZE is used to understand why a query is slow, so having to run it twice is a "waste of time", if you know what I mean :)

Example query:

ANALYZE FORMAT=JSON SELECT * FROM test_table

Thank you!

[nunoperalta]

And the same for EXPLAIN FORMAT=JSON on MariaDB

[ibennetch]

This seems like a reasonable request.

[yegongid]

Good

[nunoperalta]

@williamdes @MauricioFauth I was looking at the code here,

seems that ANALYZE maps to AnalyzeStatement, which is seen as "Table Maintenance Statements", which is fine, for ANALYZE TABLE.

However, ANALYZE SELECT is similar to EXPLAIN (in MariaDB), so it probably should be seen as ExplainStatement.
Not sure if this might complicate things here, in the code.
In MySQL, it seems the syntax is EXPLAIN ANALYZE.

--

Anyway, is this how you'd implement this Feature Request?
(in Results.php)

        } elseif (preg_match('/^\s*(EXPLAIN|ANALYZE|EXPLAIN\s+ANALYZE)\s+FORMAT\s*=\s*JSON/i', $this->properties['sql_query'])) {
            $query['pftext'] = self::DISPLAY_FULL_TEXT;

[williamdes]

Hi @nunoperalta
I think we would probably use our sql parser and no regex expression for that. The parser can indicate to us what statement this is. But like you explained it seems to be mistaken for now. I would say this is the first step to do, implement or fix the statement on the parser

[nunoperalta]

@williamdes Thanks
Something like this then?

[williamdes]

Probably, @iifawzi can I have some of your precious help please?

[iifawzi]

Yes, this definitely should be fixed in the parser, I've not really looked over all the comments, but I will take a deeper look later this weekend. side note: I've been inactive for a while, but I'm planning to get back on track, starting this weekend, hopefully.

I'm just afraid that the solution you proposed might break other parts that depend on the ANALYZE statements (without TABLE at the end), but let's have a deeper look this week.

[nunoperalta]

Thank you very much, folks.
Really appreciate your time. Cheers!

[iifawzi]

we can get benefit from the querytype property from our parser instead of using regex, we're already detecting the statements, then maybe we can check if querytype is either EXPLAIN or ANALYZE and set full text accordingly.

@williamdes @MauricioFauth I was looking at the code here,

seems that ANALYZE maps to AnalyzeStatement, which is seen as "Table Maintenance Statements", which is fine, for ANALYZE TABLE.

However, ANALYZE SELECT is similar to EXPLAIN (in MariaDB), so it probably should be seen as ExplainStatement. Not sure if this might complicate things here, in the code. In MySQL, it seems the syntax is EXPLAIN ANALYZE.

--

I didn't understand the issue mentioned here, is it a separate issue? and what its impact on this feature request. I might be missing something. btw, we have no implementation for the EXPLAIN statement yet

[nunoperalta]

@iifawzi - Thank you for looking into this.

The issue is that ANALYZE is mapping to AnalyzeStatement, under "Table Maintenance Statements".

ANALYZE SELECT ... is not a Table Management Statement.
ANALIZE TABLE is.

I'm just afraid that ANALYZE is always assumed to be a Table Management Statement, and therefore used in the code only for that purpose, making it a bit hard to differentiate between ANALYZE TABLE and ANALYZE SELECT, which are totally different things.

If you don't think that can cause an issue, then great! I'm not familiar at all with the phpMyAdmin code :)

[iifawzi]

@iifawzi - Thank you for looking into this.

The issue is that ANALYZE is mapping to AnalyzeStatement, under "Table Maintenance Statements".

ANALYZE SELECT ... is not a Table Management Statement. ANALYZE TABLE is.

I'm just afraid that ANALYZE is always assumed to be a Table Management Statement, and therefore used in the code only for that purpose, making it a bit hard to differentiate between ANALYZE TABLE and ANALYZE SELECT, which are totally different things.

If you don't think that can cause an issue, then great! I'm not familiar at all with the phpMyAdmin code :)

I got it, my bad, thank you for elaborating on this.

You're right, this's definitely an issue from the parser's point of view if the ANALYZE SELECT is seen as ANALYZE statement. I will take another look at it tomorrow

[iifawzi]

Summing this up:

1- We have ANALYZE statement in the parser, this should only parse ANALYZE TABLE statements in both MariaDB and MySQL and the parse logic for this is already implemented.
https://dev.mysql.com/doc/refman/5.6/en/analyze-table.html
https://mariadb.com/kb/en/analyze-table/

2- We have EXPLAIN statement in the parser ( not implemented yet ), this should only parse:

• Explain and Explain Analyze in MySQL.
  https://dev.mysql.com/blog-archive/mysql-explain-analyze/
  https://dev.mysql.com/doc/refman/5.7/en/explain.html
• Explain statement in MariaDB
  https://mariadb.com/kb/en/explain/
• EXPLAIN ANALYZE for old MariaDB versions and ANALYZE for versions newer than MariaDB 10.1.0.
  https://mariadb.com/kb/en/analyze-statement/
  https://mariadb.com/kb/en/explain-analyze/

That being said, All statements from the parser point of view should be detected as EXPLAIN query type, except ANALYZE TABLE statements to be detected as ANALYZE query type.

This feature request can be fixed then by checking if the query type is EXPLAIN, if true, full text should be checked automatically.

what do you think? @williamdes @nunoperalta?

[nunoperalta]

That seems spot on, @iifawzi ! Thank you very much.

Just to let you know in advance, please note that checking for ANALYZE SELECT isn't enough, as there can be a FORMAT=xxx in the middle, e.g. ANALYZE FORMAT=JSON SELECT ....

Thank you once more. Have a great weekend 👍🏼👍🏼

[williamdes]

I just merged phpmyadmin/sql-parser#386 and pushed 9a84527 this morning. So the hash should be updated and changes be made here if necessary before sql-parser is released

[iifawzi]

Perfect, I will make sure to create a PR to address the needed changes for this feature to be done

[nunoperalta]

@iifawzi - thank you very much for this!

I saw your test cases,

can you please also add another test?

ANALYZE FORMAT=JSON SELECT * FROM orders

[iifawzi]

@iifawzi - thank you very much for this!

I saw your test cases,

can you please also add another test?

ANALYZE FORMAT=JSON SELECT * FROM orders

This can surely be added, but this case is implicitly covered by the added tests already because we're checking whether it's an ANALYZE TABLE Statement or not, if it's not so this's definitely an Explain statement.

https://github.com/phpmyadmin/sql-parser/blob/8fddb4becdfb657db3b9ade52e952ef3c822a26b/tests/data/parser/parseExplain1.out#L425-L427

[nunoperalta]

Aahhh... ok, great then! Thanks very much 😊

[williamdes]

The (phpMyAdmin 5.2+snapshot) also available as a non official docker image is updated with the fixes made to sql-parser 🚀