[BEAM-9641] Support ZetaSQL DATE type as a Beam LogicalType by robinyqiu · Pull Request #11272 · apache/beam

robinyqiu · 2020-03-31T01:30:19Z

This PR adds support of all ZetaSQL (BigQuery Standard SQL) DATE functions to BeamSQL:

CURRENT_DATE
EXTRACT
DATE (constructing DATE from DATETIME not supported)
DATE_ADD
DATE_SUB
DATE_DIFF
DATE_TRUNC
FORMAT_DATE
PARSE_DATE
UNIX_DATE
DATE_FROM_UNIX_DATE

WEEK part not supported

r: @apilloud
cc: @TheNeuralBit @kennknowles

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

TheNeuralBit · 2020-04-20T16:01:12Z

What do you think about going ahead and defining the date logical type in org.apache.beam.sdk.schemas.logicaltypes? It would be useful in other contexts - for example it would give us something to map Avro's logical date type to (currently it is just overloaded with millis-instant onto DATETIME)

cc: @reuvenlax

robinyqiu · 2020-04-28T17:13:05Z

What do you think about going ahead and defining the date logical type in org.apache.beam.sdk.schemas.logicaltypes? It would be useful in other contexts - for example it would give us something to map Avro's logical date type to (currently it is just overloaded with millis-instant onto DATETIME)

Done. Thanks for the suggestion. I made the Date type a public logical type in org.apache.beam.sdk.schemas.logicaltypes and added a layer of indirection by letting SqlTypes.DATE reference it.

apilloud

LGTM

apilloud · 2020-04-30T18:12:58Z

nit: Can you clean up the style and call equals on the constant instead of logicalId (which could be null).

what about using a switch statement? Is there any style guidance on using switch on a String in java?

Done. I hope I could use a switch statement here, but unfortunately there is no constant IDENTIFIER defined in the LogicalType class. (I could add it to each concrete SQL logical type I create, but I don't think that is a good style.)

apilloud · 2020-04-30T18:13:53Z

nit: Call equals on constant to avoid null issues.

apilloud · 2020-04-30T18:33:54Z

nit: This order of equals is awesome!

apilloud · 2020-04-30T18:57:57Z

I think it is worth documenting that the Long is an offset from an epoch (and what that epoch is).

apilloud · 2020-04-30T19:08:37Z

If I'm reading the correctly, LocalDate is the in memory type (a struct) and Long is the wire format (an offset from epoch)? This conversion could be quite expensive. It appears the Calc nodes both take an offset in this case, when we start to think about performance we might need to change the in memory type to be offset based.

(If changing the in memory type is going to be difficult in the future, consider doing that now.)

That is unfortunate... but what in-memory type should we use instead? joda.time.LocalDate uses a millisecond long, do we want to add another joda dependency?
We could access the base type (wire format type) directly in SQL with Row#getBaseValue, but unfortunately Rows store logical types as the input type (in memory format type), so that wouldn't actually avoid a conversion.

I guess java.sql.Date is another option for a java type backed by millis.

We should consider not using a JVM, it adds performance overhead too. 🤓
I'm reasonably convinced the wire format is good and the conversion here is lossless, so if there isn't a easy drop-in replacement leave this as is.

I think Andrew is basically suggesting using a PassThroughLogicalType<Long> as a logical type for DATE. I think we could definitely consider this if performance becomes a problem in the future. (It's not easy to change the in-memory type for Date after it is made public, but we can easily define a new SqlDate.) For now I think we can leave it as is. It's more human readable (e.g. writing tests for DATE type in spec tests is simpler).

apilloud · 2020-04-30T19:24:30Z

Oops, forgot to include in my comments: ZetaSQL's range is much smaller than the underlying type, can you add a test or two for that? How do out of range values fail? (Also worth asking, do we need any special treatment for boundary conditions (LocalDate.MIN, LocalDate.MAX)? Probably not for now.)

robinyqiu · 2020-05-06T05:38:16Z

Ah, just realized that the previous comments were not sent out.

robinyqiu · 2020-05-06T05:41:28Z

Could you help trigger the tests again?

For the comment on range: Thanks for pointing it out. I overlooked this problem. I would like to create a separate PR to address it, along with range testing for other types as well.

apilloud · 2020-05-06T16:41:35Z

retest this please

robinyqiu · 2020-05-06T21:13:39Z

The failing test SparkPortableExecutionTest.testExecution should be unrelated to this change.

apilloud · 2020-05-07T17:34:10Z

Run Java PreCommit

apilloud · 2020-05-07T17:34:35Z

Run SQL Postcommit

robinyqiu · 2020-05-14T22:16:39Z

Rebased against master. Please run precommit tests again.

apilloud · 2020-05-14T22:36:24Z

retest this please

apilloud · 2020-05-14T23:51:49Z

retest this please

robinyqiu · 2020-05-15T18:48:23Z

Java PreCommit failed due to a build failure. Please help run again.

TheNeuralBit · 2020-05-15T18:51:12Z

Run Java PreCommit

TheNeuralBit · 2020-05-15T18:53:25Z

Run Java PreCommit

TheNeuralBit · 2020-05-18T15:29:16Z

Run Java PreCommit

TheNeuralBit · 2020-05-21T20:27:52Z

Something just occurred to me - are there any tests that use the DATE Type in an aggregation (e.g. MAX)?

I'd think that would run into the same issue I have in #11456 (processing logical types using their representation)

apilloud · 2020-05-21T20:34:14Z

Interesting question. You should probably add a test for JOIN as well, which will have a similar class of problems.

robinyqiu · 2020-05-21T20:36:30Z

are there any tests that use the DATE Type in an aggregation (e.g. MAX)?

No. Thanks for bringing this up. I think it is likely to run into the problem.

probot-autolabelerBot added extensions java sql labels Mar 31, 2020

robinyqiu changed the title ~~Support ZetaSQL DATE type as a Beam LogicalType~~[BEAM-9641] Support ZetaSQL DATE type as a Beam LogicalTypeMar 31, 2020

robinyqiu force-pushed the date-and-time branch 4 times, most recently from 3003dbb to cca562dCompare April 8, 2020 05:16

robinyqiu force-pushed the date-and-time branch from 32590e3 to 99bff44Compare April 28, 2020 17:10

robinyqiu force-pushed the date-and-time branch from 99bff44 to 7b89065Compare April 28, 2020 17:16

kennknowles requested review from TheNeuralBit and apilloud April 28, 2020 18:10

apilloud approved these changes Apr 30, 2020
View reviewed changes

robinyqiu force-pushed the date-and-time branch from 7b89065 to 49e680dCompare May 2, 2020 07:17

rahul8383 mentioned this pull request May 5, 2020
[BEAM-9840] Support for Parameterized Types when converting from HCat… #11569
Merged
4 tasks

robinyqiu force-pushed the date-and-time branch from 49e680d to fd89885Compare May 6, 2020 05:35

Support ZetaSQL DATE type as a Beam LogicalType
dce14f8

robinyqiu force-pushed the date-and-time branch from fd89885 to dce14f8Compare May 14, 2020 17:55

apilloud merged commit 47c246b into apache:masterMay 18, 2020

robinyqiu deleted the date-and-time branch June 11, 2020 23:33

Uh oh!

Conversation

robinyqiu commented Mar 31, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

TheNeuralBit commented Apr 20, 2020

Uh oh!

robinyqiu commented Apr 28, 2020

Uh oh!

apilloud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apilloud commented Apr 30, 2020

Uh oh!

robinyqiu commented May 6, 2020

Uh oh!

robinyqiu commented May 6, 2020

Uh oh!

apilloud commented May 6, 2020

Uh oh!

robinyqiu commented May 6, 2020

Uh oh!

apilloud commented May 7, 2020

Uh oh!

apilloud commented May 7, 2020

Uh oh!

robinyqiu commented May 14, 2020

Uh oh!

apilloud commented May 14, 2020

Uh oh!

apilloud commented May 14, 2020

Uh oh!

robinyqiu commented May 15, 2020

Uh oh!

TheNeuralBit commented May 15, 2020

Uh oh!

TheNeuralBit commented May 15, 2020

Uh oh!

TheNeuralBit commented May 18, 2020

Uh oh!

TheNeuralBit commented May 21, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apilloud commented May 21, 2020

Uh oh!

robinyqiu commented May 21, 2020

Uh oh!

Reviewers

Assignees

Labels

robinyqiu commented Mar 31, 2020•
edited
Loading

TheNeuralBit commented May 21, 2020•
edited
Loading