[CALCITE-2721] Support parsing of DOT + MEMBER_FUNCTION

[walterddr]

This fixes: https://issues.apache.org/jira/browse/CALCITE-2721

[zhztheplayer]

Can we avoid this lookahead? It seems NamedMemberFunctionCall() and CompoundIdentifier() share the same prefix.

[walterddr]

Yes, you are right. the lookahead at the extra <LPAREN> is the way I used to determine whether the compoundidentifier is actually a member function or simply a compound list of SqlIdentifier.

Do you have better suggestions on how to tackle this? like change the NamedMemberFunctionCall() to handle both the scenarios?

[zhztheplayer]

change the NamedMemberFunctionCall() to handle both the scenarios

Yes, I think so. Or if you prefer retaining these branches, I guess something like

(
    p = Identifier() {
        return new SqlIdentifier(p, getPos());
    }
|
    e = CompoundIdentifier()
    [
        e = NamedMemberFunctionCall(e)
    ]
)

should also work fine here.

[walterddr]

did a bit of test and turns out we dont even need the compoundidentifier since we need to support all simple identifier first. removing the last rule

[zhztheplayer]

Could you please check if following code works?

{
     (
        e = NamedMemberFunctionCall()
     |
         p = Identifier() {
             return new SqlIdentifier(p, getPos());
         }
     )
     { return e; }
 }

The file Parser.jj tends to guarantee to generate a near LL(2) parser, any change should aways avoid syntactic lookahead if possible.

[walterddr]

unfortunately this doesn't work. I actually tried to eliminate all these by just having this piece of code, (which I think is the right way to approach)

(
    e = //  Identifier() or CompoundIdentifier()
    [
        // ... (match <LPAREN> and then parse function arguments)
        e = createCall( ...
    ]
)

The actual problem here is that,

1. if using Identifier(), the function hierarchy is not correct. for example, foo().complex.function.name(arg1, arg2) will be resolved as ((foo().complex).function).name(arg1, arg2) which should've been foo().complex.function.name(arg1, arg2) according to the JIRA discussion.
2. if using Compoundidentifier(), the following doesn't work: foo()."EXPR$1"."EXPR$2" would not be resolved correctly as the part ."EXPR$1"."EXPR$2" is not a valid compound identifier.

One thing I can think of is. there's no way to distinguish the boundary of foo().complex.column.complex.function(...) during parser phase. If we do not accept complex function name, I think the above solution with simply just Identifier() should work.

[walterddr]

Seems like the problem about identifier can be fix once #958 is merged. I would suggest we hold off this PR until CALCITE-2674 is fixed.

[walterddr]

#958 has been merged. thus I updated the RowExpressionExtension based on SimpleIdentifier approach. Please kindly take a look @zhztheplayer @julianhyde .

One problem remains:

• tbl.col.bar is parsed as: `TBL`.`COL`.`BAR`
• tbl.foo(0).col.bar is parsed as: ((`TBL`.`FOO`(0).`COL`).`BAR`)

I am planning to fix this on CALCITE-2906.
If we think we should only support compound identifier with member function I can also alter this PR to do so.

[zhztheplayer]

Firstly, thank you for keeping working on this, @walterddr!

Comment of this line:
As the expression is written as myColumn.func(...), shouldn't it be parsed to a DOT expression? But by looking into the generated parse tree I think the result is more like a schema.func(...) (see following).

[zhztheplayer]

Or is this the problem should be resolved with CALCITE-2906?

[walterddr]

Yes, this is something I wasn't absolutely sure. Generally speaking member function should come after a record type. However, during parsing time there's really no way to distinguish.

CALCITE-2906 is more broader discussion on this (not just member function, but also affects DOT operator after array element, expression, etc.

[walterddr]

FYI, at this moment, both array element and expression only support SimpleIdentifier, not compound identifier. so I think it is better to address them together in a different PR. But I am open to any suggestions :-)

[zhztheplayer]

Thank you for your explanation, I too think it's hard to distinguish them now.

As for the array element part, do you already have a plan? I guess it maybe better to somehow move <DOT> RowExpressionExtension() ... syntax code to Expression2(), for keeping Expression2b() clean.

[michaelmior]

@walterddr Could you take a look at the conflict that has been introduced?

[walterddr]

@michaelmior thanks for the reminder. I just rebased and updated the change. I think the discussion has pretty much finalized. Can you take another look if possible?

[michaelmior]

@zhztheplayer Do you mind having a final look?

[zhztheplayer]

Thanks @walterddr and @michaelmior, I think it's good to go.

A minor suggestion - as parser might go into conflict when parsing member functions or regular functions after applying the PR, we'd better to push CALCITE-2906 forward earlier, say, we can mark that one as 1.21.0, so that we don't forget to come up with a final solution.

[walterddr]

@zhztheplayer I already have an experiment on CALCITE-2906 and seems like the concern is less obvious after I rebased with latest master. but you are right the risk is there. I will also closely involve with CALCITE-2906 and agree it should be fix soon.