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

Refactor SQL() methods #173

Merged
merged 6 commits into from
Dec 18, 2024

Conversation

apstndb
Copy link
Contributor

@apstndb apstndb commented Oct 23, 2024

This PR refactors SQL() methods.

  • This PR only modifies ast/sql.go
  • The correctness of the refactoring is ensured by not making any modifications to testdata.
  • Some of the current implementation is incorrect, so they will be fixed in a separate PR.
  • It is a part of Refactor SQL(), Pos(), End() #151

Note:
There is a idea to introduce text/template style conditions.

https://pkg.go.dev/text/template#hdr-Actions

{{if pipeline}} T1 {{end}}
	If the value of the pipeline is empty, no output is generated;
	otherwise, T1 is executed. The empty values are false, 0, any
	nil pointer or interface value, and any array, slice, map, or
	string of length zero.
	Dot is unaffected.

It can simplify more, but empty slice, map handling may require to use reflect because empty slice/map are not the same with zero values, and their types doesn't satisfy comparable.

Added: I have implement only zero value as false, but it seems not useful than expected: 1606256

ast/sql.go Outdated
}
sql += ")"
return sql
// TODO: len(s.Fields) > 0 is better than s.Fields != nil, but it need to update current testdata.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We intentionally distinct STRUCT and STRUCT<> because the first means a struct having any fields, and the second is the struct with no fields. For instance, select CAST(struct(1) AS STRUCT<x INT64>).x is valid, but select CAST(struct<>(1) AS STRUCT<x INT64>).x is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your explanation.
I understood your intention.

We intentionally distinct STRUCT and STRUCT<> because the first means a struct having any fields, and the second is the struct with no fields. For instance, select CAST(struct(1) AS STRUCT<x INT64>).x is valid, but select CAST(struct<>(1) AS STRUCT<x INT64>).x is invalid.

My thought is that:

  • It is StructLiteral, not StructType, so there is no semantic difference between STRUCT<>() and STRUCT(). It can be normalized without ambiguity.
  • STRUCT<>() is valid, but it can feel unfamiliar because it has no fields to name or type.
  • The official docs describe only STRUCT().

It might be a good idea to comment that both STRUCT() and STRUCT<>() are lexically preserved.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, we need to split the current StructLiteral into a new two s.t. TypedStructLiteral and TypelessStructLiteral for clarity.

Copy link
Contributor Author

@apstndb apstndb Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for splitting.

In ZetaSQL, both of them are treated in ASTStructConstructorWithKeyword, but they can't be mixed.

  • In typed struct syntax

    AS alias is not allowed on the input expressions.

    STRUCT<x int64>(5 AS x) Error - Typed syntax does not allow AS

Note:

@apstndb apstndb marked this pull request as ready for review October 27, 2024 13:57
@apstndb apstndb requested a review from makenowjust October 27, 2024 13:58
@apstndb
Copy link
Contributor Author

apstndb commented Oct 27, 2024

Thought: The existing SQL() definitions don't use token.Pos.Invalid() without negation, so why is there no token.Pos.Valid()?

@apstndb apstndb mentioned this pull request Oct 27, 2024
2 tasks
Copy link
Collaborator

@makenowjust makenowjust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. Thank you.

@makenowjust makenowjust merged commit 1048614 into cloudspannerecosystem:main Dec 18, 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