Skip to content

[BEAM-9179] Move value conversion logic out of ExpressionConverter - #12507

Merged
robinyqiu merged 2 commits into
apache:masterfrom
robinyqiu:refactor
Aug 10, 2020
Merged

[BEAM-9179] Move value conversion logic out of ExpressionConverter#12507
robinyqiu merged 2 commits into
apache:masterfrom
robinyqiu:refactor

Conversation

@robinyqiu

@robinyqiurobinyqiu commented Aug 8, 2020

Copy link
Copy Markdown
Contributor

[Simplification] Type translation code refactor:

  1. Move ZetaSQL => Calcite value translation logic from ExpressionConverter to ZetaSqlCalciteTranslationUtils
  2. Move helper methods for ZetaSQL => Calcite time-related value translation from DateTimeUtils to ZetaSqlCalciteTranslationUtils
  3. Rename methods in ZetaSqlCalciteTranslationUtils and ZetaSqlBeamTranslationUtils to be simpler; and inline excessive helper methods

r: @apilloud

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
---Build Status---
XLangBuild Status---Build Status---Build Status---

Pre-Commit Tests Status (on master branch)

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

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

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels

See CI.md for more information about GitHub Actions CI.

@robinyqiurobinyqiu changed the title [WIP] Move value conversion logic out of ExpressionConverter[BEAM-9179] Move value conversion logic out of ExpressionConverterAug 8, 2020

@apilloudapilloud 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, thanks for the cleanup!

String name = fields.get(i).getName();
if ("".equals(name)) {
name = "$col" + i;
name = "$col" + i; // empty field name is not allowed, generate an index-based name for it

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.

I believe a single column with an empty name is allowed. ZetaSQL allows multiple columns without names.

@robinyqiurobinyqiuAug 10, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah this comment is not accurate. The real problem is Beam does not allow multiple columns with the same name (empty in this case). Thanks for spotting this.

@robinyqiu
robinyqiu merged commit af1ce64 into apache:masterAug 10, 2020
@robinyqiu

Copy link
Copy Markdown
ContributorAuthor

cc: @ZijieSong946 This recent refactor caused a merge conflict for your WIP TIMESTAMP PR.

@ZijieSong946

Copy link
Copy Markdown
Contributor

cc: @ZijieSong946 This recent refactor caused a merge conflict for your WIP TIMESTAMP PR.

Ack.


private static RexNode structValueToRexNode(Value value, RexBuilder rexBuilder) {
return rexBuilder.makeCall(
toCalciteStructType(value.getType().asStruct(), false, rexBuilder),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This LOC specified the return STRUCT type explicitly, so Calcite does not need to infer. Adding this line introduced a bug, because later when the schema is translated to Beam Schema, duplicate field names will be caught. #12550 fixes this bug.

@apilloud

@robinyqiu
robinyqiu deleted the refactor branch August 14, 2020 20:21
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.

3 participants

@robinyqiu@ZijieSong946@apilloud