Moor: SqlParser: derive tables involved in a query

[mqus]

Hi! I looked at your sqlparser library and it seems to be very powerful. As you suggested in https://github.com/vitusortner/floor/issues/94 it fits perfectly well to derive the resulting types of a query and I will/want to integrate it into floor. But there is one more thing I would like to do with sqlparser and that is deriving the tables/views that are involved in a query. I would then want to have two groups of tables which are identified:

1. Tables the query directly depends on. Meaning if the data in one of those dependencies changes, the query could then have different results.
2. The tables which are directly altered by the query (especially for delete/update/insert queries)

With the knowledge of the engine (foreign key relations, database views, maybe trigger queries) it would then be possible to

1. Derive the tables the query indirectly depends on.

This would allow us to update streaming queries more precisely.

I would like to limit the scope a little bit to queries that change the data and not the scheme of the database. So CREATE/DROP statements would be out of scope as well as ALTER TABLE etc.

With this outlined functionality, would you be interested in having it in the sqlparser?
Do you think this is possible? Are there some pieces missing before I could implement this?

If yes, where should I start implementing this?

I would like to try adding this functionality, but I can't promise anything.

[simolus3]

To find tables involved in a query, you could use something like these visitors. Moor uses them to find out which tables a query reads from and which tables it updates. You can use them like this:

final engine = SqlEngine(); // add tables, etc.
final context = engine.analyze('INSERT INTO foo SELECT * FROM bar');

final tableDetector = UpdatedTablesVisitor()..visit(context.root);
// tableDetector.writtenTables contains foo with UpdateKind.insert
// tableDetector.foundTables bar

The visitors aren't part of the sqlparser package because I thought this was mainly a moor-specific functionality. I can move those to sqlparser if you need them as well.

Derive the tables the query indirectly depends on.

This is also something I'm working on for moor. We essentially create a graph to propagate table updates, so that we can say "an insert on table x causes an update on table y and a delete on table z". moor_generator does that here, but it uses a moor-specific representation of triggers and tables, not the Tables you'd use for sqlparser. So far we only consider triggers but I want to add support for foreign key constraints as well (#528).
Neither sqlparser or moor support database views at the moment, but it shouldn't be much work to parse them in sqlparser if you need that.

[mqus]

Thank you for the overview!

The visitors aren't part of the sqlparser package because I thought this was mainly a moor-specific functionality. I can move those to sqlparser if you need them as well.

This would be nice!

Neither sqlparser or moor support database views at the moment, but it shouldn't be much work to parse them in sqlparser if you need that.

Then this would be something I could start with, right? We will also need them for the type derivation.

[simolus3]

Then this would be something I could start with, right?

If you want to expand the parser to support CREATE VIEW statements that'd be great! I always appreciate contributions, let me know if you need anything.

I'll move the visitors to the the sqlparser package.

[mqus]

If I understood you correctly then this issue is pretty much finished now, as deriving tables seems to be pretty specific to moor and most of the work can be done with the ~AffectedTablesVisitor~ ReferencedTablesVisitor and the UpdatedTablesVisitor now. Is that right? I have a few more questions regarding the sqlparser semantics though, should I ask them here or in the chat (gitter)?

[simolus3]

Yes. I'll close this, but feel free to ask more questions here.

[mqus]

Then the first one: Does a resolved Basictype.nullType mean a type which is literally only null or also a type which could not be resolved?

[simolus3]

It's reserved to the NULL literal in practice. A type that can't be resolved will be exposed through ResolveResult.unknown being true.

[mqus]

A second one: If I analyze an UPDATE xyz query with xyz being an unknown table, sqlparser currently throws an error, because resolveColumns is called on a null value.
https://github.com/simolus3/moor/blob/6b2bd27d4dddfe14af793c016468429879a6ae59/sqlparser/lib/src/analysis/steps/column_resolver.dart#L67-L71
I was expecting to get errors only through the AnalysisContext.errors getter. Is this a bug? I tried to patch it by simply adding

    if (table==null){
      // further analysis is not really possible without knowing the table
      super.visitUpdateStatement(e, arg);
      return;
    }

But that simply leads to the ReferenceResolver throwing errors. Do you have a suggestion?

[simolus3]

Thanks for the report, this is indeed a bug. It should be fixed with c342b29b339852fe29295903f845ff58ade3822c.

[mqus]

I just noticed that the parser also throws its errors, instead of only adding them to the list. Is this intended behaviour? After all the parsing probably can't reasonably continue after encountering a wrong token. But then what is the errors list for?
https://github.com/simolus3/moor/blob/6b2bd27d4dddfe14af793c016468429879a6ae59/sqlparser/lib/src/reader/parser/parser.dart#L132-L137

[simolus3]

We use that for error recovery in the parser. When we parse multiple statements (say in a CREATE TRIGGER definition), we catch those errors and fast-forward to the next semicolon. The recovery is really simple at the moment, but it allows us to report multiple parsing errors in some scenarios.

But it's inconsistent that those exceptions sometimes bubble up and sometimes don't. As of ccea0a5d365816efe9eaeba7d8e4ed368581770a, they will always be reported through ParseResult.errors (or through the AnalysisContext when using SqlEngine.analyze directly).

[mqus]

I just noticed a small typo in the parser error messages:
https://github.com/simolus3/moor/blob/e21163d90ae4ffd59dfc8c570795eec2e80fdcc9/sqlparser/lib/src/reader/parser/parser.dart#L33
I think that } should not be there :)

[simolus3]

Thanks! Fixed in 74caeced85a8478f7f48c982e39fcd6f63c8a653

[mqus]

Another one (tell me if I should report the bugs differently :wink: ):
The resulting type of all binary operators apart from || should return BasicType.real(EDIT: int is wrong) and not the resulting type of the parents.
I just tried to add some strings (just to see what happens) and while sqlparser said that it will return a String, sqlite actually returned an integer. This does match with the sqlite spec:

The result of any binary operator is either a numeric value or NULL, except for the || concatenation operator which always evaluates to either NULL or a text value.

[simolus3]

tell me if I should report the bugs differently :wink:

Feel free to open a new issue for non-minor things this, but just commenting here is fine too.

I changed the behavior for +, -, * and / in 800ee8ac3ad78818f32a0dcf4ade498323e9f2ab. Binary operators should have worked before that.