Skip to content

[BEAM-4622] Makes required to call Beam SQL expressions validation#5912

Merged
aromanenko-dev merged 2 commits intoapache:masterfrom
aromanenko-dev:BEAM-4622-BeamSQL-expressions
Jul 17, 2018
Merged

[BEAM-4622] Makes required to call Beam SQL expressions validation#5912
aromanenko-dev merged 2 commits intoapache:masterfrom
aromanenko-dev:BEAM-4622-BeamSQL-expressions

Conversation

@aromanenko-dev
Copy link
Contributor

Moved huge switch statement block into separate method which returns new object of BeamSqlExpression and then that object calls accept() method to perform a validation. This refactoring discovered some test failures that were fixed by fixing accept() implementation of corresponding expressions.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • 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.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
--- --- --- ---

@aromanenko-dev aromanenko-dev force-pushed the BEAM-4622-BeamSQL-expressions branch from d5ee892 to b7956ad Compare July 10, 2018 13:15
@aromanenko-dev
Copy link
Contributor Author

retest this please

public boolean accept() {
// Interval types will be already converted into BIGINT after evaluation.
SqlTypeFamily opTypeFamily = opType(0).getFamily();
if (opTypeFamily.equals(SqlTypeFamily.INTERVAL_DAY_TIME)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good to add a unit test for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vectorijk Actually, it's already implicitly tested by BeamSqlDslSqlStdOperatorsTest.testTimestampDiff() and it failed after I did a refactoring of BeamSqlFnExecutor and before adding this check. Do you think it will make sense to add specific test for this case?

@aromanenko-dev aromanenko-dev force-pushed the BEAM-4622-BeamSQL-expressions branch from b7956ad to 7cf8f6c Compare July 11, 2018 13:44
@aromanenko-dev
Copy link
Contributor Author

@akedin Could you take a look on this?

@akedin
Copy link
Contributor

akedin commented Jul 16, 2018

LGTM

@aromanenko-dev aromanenko-dev merged commit 03c84dd into apache:master Jul 17, 2018
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
- Move docs from 'docs/firestore' into 'firestore/docs' and leave symlink.
- Harmonize / DRY 'firestore/README.rst' and 'firestore/docs/index.rst'.
- Remove docs for GAPIC-generated bits (they aren't part of the surface).
- Ensure that docs still build from top-level.

Toward apache#5912.
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.

3 participants