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

feat(spanner/spansql): support EXTRACT #5218

Merged
merged 7 commits into from Dec 16, 2021

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Dec 15, 2021

This adds support for EXTRACT func and similar constructions

Fixes: #5209

@rahul2393 rahul2393 requested review from a team as code owners Dec 15, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Cloud Spanner API. label Dec 15, 2021
@rahul2393 rahul2393 requested review from hengfengli and olavloite Dec 15, 2021
if err := p.expect("FROM"); err != nil {
return nil, err
}
e, err := p.parseTableOrIndexOrColumnName()
Copy link
Contributor

@olavloite olavloite Dec 15, 2021

Choose a reason for hiding this comment

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

This is formally not correct, but I'm fine with this being a limitation (that we in that case should add to the list of limitations): The definition of the EXTRACT function specifies that any timestamp expression may be given at this position. That also includes any timestamp literal or function call that returns a timestamp. This implementation only allows the use of a column that contains a timestamp.

So it means that the following statement would not parse correctly:

select extract(date from timestamp '2020-01-01T10:00:00+01:00' at time zone 'Europe/Amsterdam') as d

if tok.err != nil {
return nil, err
}
return TypedExpr{Expr: Func{Name: "AT TIME ZONE", Args: []Expr{e, StringLiteral(tok.string)}}, Type: partType}, nil
Copy link
Contributor

@olavloite olavloite Dec 15, 2021

Choose a reason for hiding this comment

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

This is modelling AT TIME ZONE as a function. I don't think that is correct (or at least it will be very difficult to work with when you want to implement it), as function names do normally not contain spaces. Instead, I think this should be considered as a timezone expression, and then be passed in as an optional argument to the EXTRACT function.

In order to get an idea of what works best, it might be good to add (an) empty implementation(s) of the function(s) in spannertest, and add tests for that as well. The implementation(s) can just return a fixed value, but then you know that you have chosen a parsing strategy that will work.

@@ -524,7 +528,9 @@ func TestParseDDL(t *testing.T) {
CREATE TABLE users (
user_id STRING(36) NOT NULL,
some_string STRING(16) NOT NULL,
some_time TIMESTAMP NOT NULL,
Copy link
Contributor

@olavloite olavloite Dec 15, 2021

Choose a reason for hiding this comment

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

nit: fix indentation

number_key INT64 AS (SAFE_CAST(SUBSTR(some_string, 2) AS INT64)) STORED,
generated_date DATE AS (EXTRACT(DATE FROM some_time AT TIME ZONE "CET")) STORED,
Copy link
Contributor

@olavloite olavloite Dec 15, 2021

Choose a reason for hiding this comment

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

nit: fix indentation

Copy link
Contributor

@olavloite olavloite left a comment

LGTM

func (aze AtTimeZoneExpr) SQL() string { return buildSQL(aze) }
func (aze AtTimeZoneExpr) addSQL(sb *strings.Builder) {
aze.Expr.addSQL(sb)
sb.WriteString(" AT TIME ZONE ")
Copy link
Contributor

@olavloite olavloite Dec 15, 2021

Choose a reason for hiding this comment

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

super nit: remove extra space

Suggested change
sb.WriteString(" AT TIME ZONE ")
sb.WriteString(" AT TIME ZONE ")

@rahul2393 rahul2393 merged commit 81b7c85 into googleapis:main Dec 16, 2021
4 checks passed
BrennaEpp pushed a commit to BrennaEpp/google-cloud-go that referenced this pull request Dec 23, 2021
* feat(spanner/spansql): support EXTRACT

* added separate Expr for Extract func and added unit and integration tests

* add test for year

* repleace atTimeZone func with atTimeZone expression

* fixing failing tests

* added negative test, reduced the valid extract part values.

* remove extra space

Co-authored-by: Rahul Yadav <irahul@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Cloud Spanner API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner/spansql: v1.28.0 generated columns
2 participants