Why are we stopping at logical plan here? Can we go all the way to local physical planning?

I think we can still check at logical planning, but also check at local physical planning after that.

The statement parser tests are focused on parsing, only.

This PR does not attempt to plug qualifiers into commands just yet, to keep the scope reasonable. That's the next step in #133068. In the next step, we definitely want to have tests that inspect the generated logical and physical plans more closely.

The original PoC plugs everything in end-to-end, which is a big change, and I'm (roughly) following the PoC by splitting out parts into smaller PRs that we can iterate on more easily.

Maybe I missed them but I did not see any tests with escaping. Can we add the following tests if not already there?
A field containing left bracket (first, middle, last) [field, fiel[d, field[
A field containing right bracket (first, middle, last) ]field, fiel]d, field]
An index name containing left bracket (first, middle, last)
An index name containing right bracket (first, middle, last)
An field name containing dot (first, middle, last) .field, fiel.d, field.
An index name containing dot (first, middle, last)

I would like to address quoting, escaping, validating against weird characters etc. in a follow-up PR. This is subtle enough that it deserves a bunch of comments and some discussions.

I noted in #133068 that we want to disallow everything but very basic qualifiers at first.

I'd also really disallow dots in qualifier names for now unless really needed. Let's avoid doing the same mistake twice.

I wonder if simplifying the grammar, reducing the scope to allow only qualifier = UNQUOTED_IDENTIFIER for qualifiedNamePattern would make it a bit easier as the first step. If I understand the intention correctly here, we want to allow patterns in qualifiers, as we allow patterns on index/field/attribute names.

For example, allowing the query below to be valid

FROM test
LOOKUP JOIN test t1 on x
LOOKUP JOIN test t2 on y
DROP [t*].[z*]

As far as I know, SQL does not use * in qualifiers to represent multiple tables. As qualifier is new, and we have the control of its scope, expanding a scope is easier than shrinking it, as the later could be a breaking change potentially.

Alternatively, if we really want to allow wildcard in the qualifier, will qualifier=UNQUOTED_ID_PATTERN work? Does the qualifiers need to be quoted? As it looks like the qualifiers defined in Join is UNQUOTED_SOURCE/UNQUOTED_IDENTIFIER.

I wonder if simplifying the grammar, reducing the scope to allow only qualifier = UNQUOTED_IDENTIFIER
I wonder if simplifying the grammar, reducing the scope to allow only qualifier = UNQUOTED_IDENTIFIER

I wish the same! Unfortunately, when trying this I realized that you cannot have this here because he lexer cannot distinguish between UNQUOTED_IDENTIFIER and ID_PATTERN , and ID_PATTERN is required to make the rest of KEEP/DROP work correctly.

As qualifier is new, and we have the control of its scope, expanding a scope is easier than shrinking it, as the later could be a breaking change potentially.

Absolutely. The allowed characters for qualifiers have to be restricted, both in patterns and in non-patterns. But let's do that in a separate PR; it's already tracked in the meta issue #133068.

Alternatively, if we really want to allow wildcard in the qualifier, will qualifier=UNQUOTED_ID_PATTERN work?

UNQUOTED_ID_PATTERN is a fragment. I didn't want to risk confusing the lexer and breaking KEEP/DROP by making it a token. I think making it a token would introduce ambiguity into the lexing of KEEP/DROP commands as it would conflict with ID_PATTERN.

If the quoted pattern is not allowed in the grammar, perhaps this check can be removed.

See above - I don't want to mess up the lexer.

An alternative approach that I can think of is limiting the scope of qualifiers in the grammar where the qualifiers are not supported, where we throw these qualifiers unsupported exceptions - in the grammar, use fieldName and fieldNamePattern instead of qualifiedName and qualifiedNamePattern.

Both approaches need multiple lines of changes though, I'm not quite sure one is easier than the other. The advantage of this approach is that we can get more meaningful error messages in the ParsingException, the advantage of limiting it in the grammar is that it could help auto-completion to detect the unsupported places earlier from the grammar errors.

Just want to bring this up, so that we are aware of these options, I'm fine with either of them.

• in the grammar, use fieldName and fieldNamePattern instead of qualifiedName and qualifiedNamePattern.

Oh yes! Just using fieldName and fieldNamePattern in commands is much cleaner.

But I did not want to make a large scale change to the grammar while this is still so in flux. I'll add it to #133068, though, because this approach is less brittle.

This is a quiet verbose syntax that requires wrapping an identifier with brackets rather than just pre-pending it with qualifier when changing existing query.

I wonder about the following alternatives:

• [qualifier].identifier
• qualifier -> identifier

They are a bit shorter. Could they be ambiguous?

This PR's syntax may not be the final one. That said, I think verbose and unambiguous is fine because we plan to introduce shorthands when unambiguous: qualifier.identifier and just identifier.

I personally like [qualifier].identifier as well, but received feedback not to use -> in case we want to introduce lambdas into the language at some point in time.

Does back implies we had them at some point before?

Yes! They were removed in #127854.