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

Support proto/enum as typename #121

Merged

Conversation

apstndb
Copy link
Contributor

@apstndb apstndb commented Sep 29, 2024

This PR adds NamedType.

it is required for PROTO/ENUM support:

@makenowjust
Copy link
Collaborator

I'm concerned that it seems to be abusing Path for the field of NamedType. Path is a type for path expressions.

@apstndb
Copy link
Contributor Author

apstndb commented Sep 29, 2024

OK. NamedType will have a raw []*Ident.

@apstndb
Copy link
Contributor Author

apstndb commented Sep 29, 2024

FYI
In ZetaSQL, SELECT AS a.b.c is parsed as PathExpression node, so it is
https://github.com/google/zetasql/blob/194cd32b5d766d60e3ca442651d792c7fe54ea74/zetasql/parser/testdata/select_as_distinct_all.test#L239-L243

(Added) CAST(expr AS typename) is parsed as CastExpression(expr, SimpleType(PathExpression)...). There is no difference between native types and proto types.
https://github.com/google/zetasql/blob/194cd32b5d766d60e3ca442651d792c7fe54ea74/zetasql/parser/testdata/parser.test#L1194-L1264

@makenowjust
Copy link
Collaborator

makenowjust commented Sep 29, 2024

It is abnormal that the struct with the suffix Expression is used as a type, so ZetaSQL's design is absolutely wrong.

@apstndb
Copy link
Contributor Author

apstndb commented Sep 29, 2024

I think there is a confusion.

In GoogleSQL of Cloud Spanner(and ZetaSQL), path expression is a structure for dot-separated identifiers described in lexical structure document, and it is used in many places of ZetaSQL.
https://cloud.google.com/spanner/docs/reference/standard-sql/lexical#identifier_examples
https://cloud.google.com/spanner/docs/reference/standard-sql/lexical#path_expressions

path:
  [path_expression][. ...]

path_expression:
  [first_part]/subsequent_part[ { / | : | - } subsequent_part ][...]

(It seems they call path as "path expression" in many situation.)

Generally, expr.expr can be parsed as field access operator GoogleSQL expression.
https://cloud.google.com/spanner/docs/reference/standard-sql/operators#field_access_operator.

Question: Why do we care about a path expression being a GoogleSQL expression?

@makenowjust
Copy link
Collaborator

A: Use a field Path []*Ident for that purpose like in this PR and UpdateItem.

Path []*Ident // len(Path) > 0

@apstndb
Copy link
Contributor Author

apstndb commented Sep 29, 2024

I think many nodes need dot-separated identifiers to implement named schemas.
Should all they have raw []*Ident?

@apstndb
Copy link
Contributor Author

apstndb commented Sep 29, 2024

I think it is similar in that *ast.Ident is a ast.Expr but it is used in many places.

@makenowjust
Copy link
Collaborator

I'm sorry. I will explain the essential issue. Path is a list of dot-chained identifiers, where len(Idents) >= 2 is a constraint. However, your use of it this time clearly violated it. Then, you needed to fix it. I feel it's my fault for not explaining that this is the biggest problem, and the GoogleSQL/ZetaSQL situation is not important here.

In the future, if ASTs similarly appear frequently where len(Idents) > 0 (len(Path) > 0) is a constraint, then it is worth naming such a structure. Also, we could consider using Path (even if it is not an expression) if we see a dot-chained list in the syntax where len(Idents) >= 2.

@apstndb
Copy link
Contributor Author

apstndb commented Sep 30, 2024

Thank you for explaining.
We will discuss this topic another time. (It would be in named schema #100).

I think there are no reason to have deeper hierarchy as NamedType{Path{[]*Ident}}, so I agree NamedType should have []*Ident simply.

@makenowjust makenowjust merged commit cdd31d5 into cloudspannerecosystem:main Oct 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants