refactor: consolidate table_reference and invocation identifier into object_reference #150

dmfay · 2023-05-25T04:19:58Z

Incorporates #149 ; not ready for primetime yet, there are four test failures that mostly stem from incorrect precedence negotiation between object_reference and field -- give it a whirl if you have time!

@matthias-Q if this is too much (it's a lot) you can avoid a lot of the precedence-setting in #149 by setting it in parametric_type as here. It still wants the precedence on relation but I haven't gone too far down that rabbit hole yet.

matthias-Q

Looks good. I like the new _dml_* structure

matthias-Q · 2023-05-25T06:05:26Z

grammar.js

+            '.',
+          ),
+        ),
+        choice(


this is a choice of just one element.

matthias-Q · 2023-05-25T17:58:30Z

grammar.js

@@ -625,7 +642,7 @@ module.exports = grammar({
      optional(
        $.keyword_only,
      ),
-      $.table_reference,
+      $.object_reference,


We should not forget to fix the queries downstream in nvim-treesitter. There is a query matching on table_reference

matthias-Q · 2023-05-25T18:17:14Z

The test Update with a JOIN is failing, because the identifier is missing the schema. In the test the assignment is written as a.d = 5;

    assignment: $ => seq(
      field('left', $.identifier), // column name only, no schema/table qualifiers
      '=',
      field('right', $._expression),
    ),

I do not understand why this has been changed from field to identifier

matthias-Q · 2023-05-25T19:59:21Z

I have a variant of your branch, where only two tests fail (Simple select with schema and fully-pathed fields, insert with field name). Should I push it to this PR?

dmfay · 2023-05-25T22:56:27Z

I have a variant of your branch, where only two tests fail (Simple select with schema and fully-pathed fields, insert with field name). Should I push it to this PR?

yes absolutely!

I do not understand why this [update assignment left-hand side] has been changed from field to identifier

I didn't follow up on MySQL multi-table updates (where the lhs can be qualified) until later and forgot to undo that change. I'll wait to change anything until you've pushed your updates though.

matthias-Q · 2023-05-26T05:12:12Z

Not so many changes. I played around with your changes a bit.

The last two tests are breaking because of L1308 - _set_values is commented out. When adding this is a whole lot more breaks. I assume this is what you are referring to with the MySQL updates.

The other one is the fully qualified object_ref. LL1260 object_reference only has schema.name and it should be schema.table.col. But there I get a lot of issues.

[Edit]: I did not comment all in in L1308. Now it is just one test failing, and thats the fully qualified path fields.

matthias-Q · 2023-05-26T05:49:07Z

Now I stumbled over your comments before _set_value. There is this test (Insert with field name) and it is valid in MySQL. I was not able to find it in the documentation but I did a quick test against a mysql instance.

dmfay · 2023-06-01T03:31:34Z

some experimenting here in a toy subset https://github.com/dmfay/tsqltest

this gets sch.tbl.field, sch.tbl.* (missing a test in main atm!), etc correctly in select clauses but breaks join predicates, order columns, and so on when transferred to the full tree-sitter-sql since field no longer contains an internal object_reference

I think it is promising or at least a hopeful sign though! I'll keep looking into this

failures now are from tree-sitter resolving multiple values, e.g a binary_expression with a qualified lhs winds up with multiple `left` value attributes

dmfay · 2023-06-19T19:54:21Z

okay I'm alive! Lots of progress here -- actually the object_reference move is complete, and the remaining issue I can see is with the various optimize statements added in the interim. As pushed this doesn't compile, complaining about conflicts first in vacuum and then in others.

The vacuum conflict is resolved with prec.right but that's a very blunt instrument and there's something extra-fishy going on: one of the conflict paths involves field _vacuum_table_repeat1, although paren_list uses plain repeat not repeat1, and then if you change line 1490 to specify identifier the conflict now implicates create_view_repeat1 which has a paren_list($.identifier)! Possibly we need to lift those into named/anonymous rules?

matthias-Q · 2023-06-19T21:20:46Z

I don't understand why we have conflicts here at all. I mean vacuum is close to the root of the tree and it is followed by a node with is almost at the bottom (field; identifier).
From my understanding, this should not lead to any disambiguities as it is a fairly unique parsing "path".

What do you mean by "lifting into named/anonymous rules"?

dmfay · 2023-06-19T22:19:48Z

I don't understand why we have conflicts here at all. I mean vacuum is close to the root of the tree and it is followed by a node with is almost at the bottom (field; identifier). From my understanding, this should not lead to any disambiguities as it is a fairly unique parsing "path".

What do you mean by "lifting into named/anonymous rules"?

because the anomalous repeat1s reference other rules entirely, I wonder if tree-sitter is doing something strange with scopes of "shared" anonymous rules like paren_list -- that is, because a paren_list(identifier) appears in the "create view" path first or last, other instances of paren_list(identifier) get mixed up with "create view" syntax. So all's well if we have only the one instance of paren_list(identifier) but the moment we have another we need to have an explicit view_params_paren_list + other_thing_paren_list, or at least that's my current guess.

dmfay · 2023-06-20T01:39:49Z

I think it's because of how we structure programs as sequential statements and often have optional ; delimiters. So vacuum tbl ( NEXT_TOKEN could express either a single statement expressing vacuum options or two statements, a simplest-possible vacuum and a parenthesized expression.

That seems like a bit much to try to tackle here so I've added a conflict for _vacuum_table and _compute_stats which lets it compile for now. All tests pass, but we should revisit programs as sequences of delimited statements soon!

matthias-Q

This is awesome. I really like that we have a clearer structure now.

There 3 comments, two of them are just about comments in the code.

matthias-Q · 2023-06-20T06:03:25Z

grammar.js

+      // TODO i don't think we have a test for `insert into tbl set ....` and
+      // it's no dialect i can think of


If I read the docs correclty, then MariaDB allows for this:
https://mariadb.com/kb/en/insert/

matthias-Q · 2023-06-20T06:08:28Z

grammar.js

@@ -2092,26 +2101,29 @@ module.exports = grammar({
      ),
      optional($.where),
      optional($.group_by),
+      optional($.window_clause),


is is correct that the window_clause can be between group_by and order_by? Does the order matter here? I thought that is should be last.

I followed Postgres in placing it between having and order by. I haven't seen another product with a standalone window clause.

It does make sense as limit and order by apply to the result of set operations (union, intersect, except) & you can see those are a dividing line

grammar.js

matthias-Q and others added 5 commits May 24, 2023 17:50

feat: add parenthesized select and cte select statements

b645f8c

refactor: consolidate into object_reference

10fa4a0

test: update update

d772aef

fix: correct window clause placement

c67da1b

refactor: split dml read/write

d5e23cf

dmfay requested a review from matthias-Q May 25, 2023 04:19

matthias-Q reviewed May 25, 2023

View reviewed changes

fix: assignment change from identifier to field

713ff4d

fix: _set_value choice

17b3f10

matthias-Q mentioned this pull request Jun 19, 2023

ERROR on COUNT(DISTINCT tbl.field) #154

Closed

dmfay added 6 commits June 19, 2023 13:42

refactor: use term directly in select_expression, minimize field

dc991da

failures now are from tree-sitter resolving multiple values, e.g a binary_expression with a qualified lhs winds up with multiple `left` value attributes

refactor: break out _qualified_field

eea1e28

fix: alias _qualified_field into assignment

e7f1c4a

fix: support qualified all_fields

bb7d8c4

test: schema.table.*

6d635e7

Merge branch 'main' into dmf/objectref

0c9afe6

fix: kludge around statement delimiters for now

30668da

dmfay marked this pull request as ready for review June 20, 2023 01:39

matthias-Q approved these changes Jun 20, 2023

View reviewed changes

dmfay commented Jun 20, 2023

View reviewed changes

grammar.js Outdated Show resolved Hide resolved

dmfay added 2 commits June 20, 2023 08:47

Update grammar.js

52bedb8

Update grammar.js

c8ffe86

dmfay mentioned this pull request Jun 20, 2023

fix(sql): table_reference -> object_reference nvim-treesitter/nvim-treesitter#4979

Merged

dmfay merged commit 6a2d466 into main Jun 20, 2023

dmfay deleted the dmf/objectref branch June 20, 2023 22:00

dmfay mentioned this pull request Jun 22, 2023

fix: impala partitioning variation #162

Merged

ds-cbo mentioned this pull request Oct 4, 2023

bump tree-sitter-sql helix-editor/helix#8464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: consolidate table_reference and invocation identifier into object_reference #150

refactor: consolidate table_reference and invocation identifier into object_reference #150

dmfay commented May 25, 2023

matthias-Q left a comment

matthias-Q May 25, 2023

matthias-Q May 25, 2023

matthias-Q commented May 25, 2023 •

edited

Loading

matthias-Q commented May 25, 2023 •

edited

Loading

dmfay commented May 25, 2023

matthias-Q commented May 26, 2023 •

edited

Loading

matthias-Q commented May 26, 2023

dmfay commented Jun 1, 2023

dmfay commented Jun 19, 2023

matthias-Q commented Jun 19, 2023

dmfay commented Jun 19, 2023

dmfay commented Jun 20, 2023 •

edited

Loading

matthias-Q left a comment

matthias-Q Jun 20, 2023

matthias-Q Jun 20, 2023

dmfay Jun 20, 2023

dmfay Jun 20, 2023

		// TODO i don't think we have a test for `insert into tbl set ....` and
		// it's no dialect i can think of

refactor: consolidate table_reference and invocation identifier into object_reference #150

refactor: consolidate table_reference and invocation identifier into object_reference #150

Conversation

dmfay commented May 25, 2023

matthias-Q left a comment

Choose a reason for hiding this comment

matthias-Q May 25, 2023

Choose a reason for hiding this comment

matthias-Q May 25, 2023

Choose a reason for hiding this comment

matthias-Q commented May 25, 2023 • edited Loading

matthias-Q commented May 25, 2023 • edited Loading

dmfay commented May 25, 2023

matthias-Q commented May 26, 2023 • edited Loading

matthias-Q commented May 26, 2023

dmfay commented Jun 1, 2023

dmfay commented Jun 19, 2023

matthias-Q commented Jun 19, 2023

dmfay commented Jun 19, 2023

dmfay commented Jun 20, 2023 • edited Loading

matthias-Q left a comment

Choose a reason for hiding this comment

matthias-Q Jun 20, 2023

Choose a reason for hiding this comment

matthias-Q Jun 20, 2023

Choose a reason for hiding this comment

dmfay Jun 20, 2023

Choose a reason for hiding this comment

dmfay Jun 20, 2023

Choose a reason for hiding this comment

matthias-Q commented May 25, 2023 •

edited

Loading

matthias-Q commented May 25, 2023 •

edited

Loading

matthias-Q commented May 26, 2023 •

edited

Loading

dmfay commented Jun 20, 2023 •

edited

Loading