Skip to content
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

AS OF won't parse without a FROM clause. #8873

Open
nicktobey opened this issue Feb 20, 2025 · 3 comments
Open

AS OF won't parse without a FROM clause. #8873

nicktobey opened this issue Feb 20, 2025 · 3 comments
Labels
good first issue Good for newcomers parser sql Issue with SQL

Comments

@nicktobey
Copy link
Contributor

The following is a completely valid way to get the hash of a table on a chosen branch:

SELECT DOLT_HASHOF_TABLE('table') AS OF main

However, this fails to parse with the error message:

syntax error at position 36 near 'of'
select DOLT_HASHOF_TABLE('t') as of HEAD

However, SELECT DOLT_HASHOF_TABLE('table') FROM table AS OF main is accepted, despite the fact that table is not actually used in the query. Curiously, SELECT DOLT_HASHOF_TABLE('table') FROM dual AS OF main is also rejected.

This is likely a parsing issue in Vitess, where the grammar only matches an AS OF clause in the presence of a FROM clause. (And AS OF dual is handled specially.)

@nicktobey nicktobey added good first issue Good for newcomers parser sql Issue with SQL labels Feb 20, 2025
@dds05
Copy link

dds05 commented Feb 27, 2025

@nicktobey Can you please assign this to me ?

Could you provide some details on how to get started with this?
Eg. Where can I run the queries and debug the parser?

@nicktobey
Copy link
Contributor Author

Here's how I would get started:

Our parser is a fork of Vitess and is kept in a separate repository that Dolt depends on: https://github.com/dolthub/vitess

It uses yacc to generate the parsing code. The parsing grammar is all described in go/vt/sqlparser/sql.y

We have a test file go/vt/sqlparser/parse_test.go with a variable called validSQL, which is an array of test cases. For each test case, the inputcontains a SQL query that we expect to be accepted by the parser. You can add a test for the broken query. (The test will fail for now, but it will pass once you fix the issue.) Don't worry about the output field for this, don't include it in your test.

The biggest challenge for this issue is probably understanding the sql.y grammar file if you aren't already familiar with yacc.

Let me know if you have any further questions!

@nicktobey
Copy link
Contributor Author

nicktobey commented Feb 28, 2025

Actually, I'd hang off on trying to fix it. The issue is more subtle and more complicated than I originally thought, and I'm going to discuss with the team what the best solution for this is.

Basically, the reason why this gets rejected is because the AS OF clause is part of a table alias expression. That's it's job: to name a different commit or branch to use when resolving the table name.

But the DOLT_HASHOF_TABLE system function and other system functions like it don't use tables. They work entirely off of the state of the currently checked out branch.

This means that the workaround query I suggested in the first post (SELECT DOLT_HASHOF_TABLE('table') FROM table AS OF main) doesn't actually work. It modifies which version of table is referenced, but that table isn't actually used when evaluating the query. The engine wil get the hash from the currently checked-out branch, and the branch named in the AS OF clause will be ignored and have no effect on the result.

Given this, allowing AS OF to be used outside of a table alias and affect behavior other than table name resolution would be a departure from how it currently works, making this a more complicated change that might have unintended consequences.

On the other hand, I can't think of other way to get a table hash within a query. This would mean that there's currently no way to get a table hash for branches other than the checked out branch, and maybe there should be.

So @dds05, we appreciate your offer have this assigned to you, but we probably shouldn't do anything until we figure out what the correct behavior should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers parser sql Issue with SQL
Projects
None yet
Development

No branches or pull requests

2 participants