Skip to content

[BEAM-10224] Test group by and aggregation on DATE and TIME type - #12120

Merged
kennknowles merged 1 commit into
apache:masterfrom
robinyqiu:date-time
Jul 8, 2020
Merged

[BEAM-10224] Test group by and aggregation on DATE and TIME type#12120
kennknowles merged 1 commit into
apache:masterfrom
robinyqiu:date-time

Conversation

@robinyqiu

@robinyqiurobinyqiu commented Jun 29, 2020

Copy link
Copy Markdown
Contributor

r: @TheNeuralBit@kennknowles

cc: @ZijieSong946@apilloud@reuvenlax

Post-Commit Tests Status (on master branch)

LangSDKDataflowFlinkSamzaSparkTwister2
GoBuild Status---Build Status---Build Status---
JavaBuild StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build StatusBuild Status
Build Status
Build Status
Build Status
PythonBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
---Build Status---
XLang------Build Status------Build Status

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsite
Non-portableBuild StatusBuild Status
Build Status
Build StatusBuild Status
Portable---Build Status------

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

@robinyqiurobinyqiu changed the title [BEAM-10224] Test Aggregation on DATE and TIME type[BEAM-10224] Test group by and aggregation on DATE and TIME typeJun 29, 2020
@robinyqiu

Copy link
Copy Markdown
ContributorAuthor

retest this please

@robinyqiu

Copy link
Copy Markdown
ContributorAuthor

The new ZetaSQL DATE and TIME types use Beam logical types. Currently Beam SQL transforms operate on the base types of logical types. This produces incorrect result because input types are often expected. In addition, after #11074 values of input type are stored in Rows. I think we should leverage that change to avoid unnecessary conversion to base types for better performance, instead of forcing the use of base types.

@kennknowleskennknowles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I just reviewed the diffs, which seem straightforward. If there is anything else I should be thinking about LMK.

@kennknowleskennknowles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this LGTM and no further comment, I'll merge.

@kennknowles
kennknowles merged commit f65a187 into apache:masterJul 8, 2020
@robinyqiu
robinyqiu deleted the date-time branch July 17, 2020 17:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@robinyqiu@kennknowles