[BEAM-2018] refine expression of Calcite method/function by mingmxu · Pull Request #2656 · apache/beam

mingmxu · 2017-04-23T05:52:26Z

In this PR, BeamSQLFnExecutor is introduced to replace BeamSQLSpELExecutor, as the executor of SqlOperator expressions.

Several common operators are added here as reference, more are needed to cover all operators defined in org.apache.calcite.sql.fun.SqlStdOperatorTable.

coveralls · 2017-04-23T09:59:35Z

Changes Unknown when pulling 3ce5f7b on XuMingmin:BEAM-2018 into ** on apache:DSL_SQL**.

aaltay · 2017-04-24T16:48:11Z

R: @jbonofre

mingmxu · 2017-04-25T20:27:43Z

@davorbonaci can you also take a peek here?

xumingming · 2017-05-02T03:54:41Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/interpreter/operator/BeamSqlAndExpression.java

+    boolean result = true;
+    for (BeamSqlExpression exp : operands) {
+      BeamSqlPrimitive<Boolean> expOut = exp.evaluate(inputRecord);
+      result = result && expOut.getValue();


short-circuit strategy can be applied here?

+1, also for OR

xumingming · 2017-05-02T03:55:09Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/interpreter/operator/BeamSqlOrExpression.java

+    boolean result = false;
+    for (BeamSqlExpression exp : operands) {
+      BeamSqlPrimitive<Boolean> expOut = exp.evaluate(inputRecord);
+        result = result || expOut.getValue();


short-circuit strategy can be applied here

xumingming · 2017-05-02T05:25:32Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/schema/BeamSQLRow.java

+      }
    case SMALLINT:
    case TINYINT:
      if (!(fieldValue instanceof Integer)) {


Integer => Short?

xumingming · 2017-05-02T05:33:57Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/schema/BeamSQLRow.java

+        throw new InvalidFieldException(
+            String.format("[%s] doesn't match type [%s]", fieldValue, fieldType));
+      } else {
+        return Integer.valueOf(fieldValue.toString());


It seems that XXX.valueOf() calls in getFieldValue is not necessary? Because the return value of getFieldValue is Object, and since the value and type are checked in addField() method, so type cast in methods like getShort, getInteger are safe?

sounds right, it's already checked in both Constructor and addField

xumingming · 2017-05-02T06:00:27Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/schema/BeamSqlRowCoder.java

@@ -108,24 +107,24 @@ public BeamSQLRow decode(InputStream inStream, org.apache.beam.sdk.coders.Coder.

      switch (type.getFieldsType().get(idx)) {
      case INTEGER:


we should use the nested context to corresponds with the encode method?

My preferred way is NESTED...OUTER, any concern?

xumingming · 2017-05-02T06:51:21Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/interpreter/operator/BeamSqlPrimitive.java

+    case INTEGER:
+      return value instanceof Integer;
+    case SMALLINT:
+    case TINYINT:


Maping sql TINYINT to Java Byte maybe more appropriate?(Both of them are 1 byte long).

sounds a good option

xumingming · 2017-05-02T10:00:35Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/schema/BeamSqlRowCoder.java

      case VARCHAR:
        stringCoder.encode(value.getString(idx), outStream, nested);
        break;
-      case TIME:


Why Time, Timestamp related code are deleted?

It's moved to a separated task, as a hard dependency of window support in task 2006.

Changes: 1. revert BEAM dependency to 0.6.0 to avoid impact of changes in master branch; 2. updates as discussion during review; refine BeamSqlRowCoder

coveralls · 2017-05-02T17:45:44Z

Changes Unknown when pulling 1db5de8 on XuMingmin:BEAM-2018 into ** on apache:DSL_SQL**.

coveralls · 2017-05-02T18:30:46Z

Changes Unknown when pulling 1db5de8 on XuMingmin:BEAM-2018 into ** on apache:DSL_SQL**.

mingmxu · 2017-05-02T19:01:05Z

Retest this please

coveralls · 2017-05-02T20:27:45Z

Changes Unknown when pulling 1db5de8 on XuMingmin:BEAM-2018 into ** on apache:DSL_SQL**.

mingmxu · 2017-05-02T20:59:20Z

the failure should be safe to ignore.
all pass, except org.apache.beam.examples.WordCountIT.testE2EWordCount

davorbonaci

LGTM; ready for merge to a feature branch modulo a few superficial comments.

davorbonaci · 2017-05-03T21:38:57Z

dsls/pom.xml

    <groupId>org.apache.beam</groupId>
    <artifactId>beam-parent</artifactId>
-    <version>0.7.0-SNAPSHOT</version>
+    <version>0.6.0</version>


Use current version for the parent pom.

davorbonaci · 2017-05-03T21:39:15Z

dsls/sql/pom.xml

    <groupId>org.apache.beam</groupId>
    <artifactId>beam-dsls-parent</artifactId>
-    <version>0.7.0-SNAPSHOT</version>
+    <version>0.6.0</version>


Use current version for the parent pom, but instead overwrite dependencyManagement section they way you need it.

davorbonaci · 2017-05-03T21:49:06Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/exception/BeamInvalidOperatorException.java

+  /**
+   *
+   */
+  private static final long serialVersionUID = 3445015747629217342L;


let's remove all UIDs?

coveralls · 2017-05-04T00:09:02Z

Changes Unknown when pulling d98b00a on XuMingmin:BEAM-2018 into ** on apache:DSL_SQL**.

mingmxu · 2017-05-04T00:17:25Z

@davorbonaci
The test failure org.apache.beam.examples.WordCountIT.testE2EWordCount is still there, I think an sync-up from beam/master is required after that.

mingmxu · 2017-05-04T15:33:01Z

Retest this please

coveralls · 2017-05-04T16:56:12Z

Changes Unknown when pulling d98b00a on XuMingmin:BEAM-2018 into ** on apache:DSL_SQL**.

xumingming reviewed May 2, 2017

View reviewed changes

mingmxu force-pushed the BEAM-2018 branch from 6f13579 to db3fd9e Compare May 2, 2017 17:05

redesign BeamSqlExpression to execute Calcite SQL expression.

1db5de8

Changes: 1. revert BEAM dependency to 0.6.0 to avoid impact of changes in master branch; 2. updates as discussion during review; refine BeamSqlRowCoder

mingmxu force-pushed the BEAM-2018 branch from db3fd9e to 1db5de8 Compare May 2, 2017 17:11

davorbonaci approved these changes May 3, 2017

View reviewed changes

update pom.xml, and remove all UIDs

d98b00a

asfgit pushed a commit that referenced this pull request May 4, 2017

This closes #2656

a6845a3

mingmxu closed this May 4, 2017

mingmxu deleted the BEAM-2018 branch May 4, 2017 17:01

		@@ -108,24 +107,24 @@ public BeamSQLRow decode(InputStream inStream, org.apache.beam.sdk.coders.Coder.

		switch (type.getFieldsType().get(idx)) {
		case INTEGER:

Conversation

mingmxu commented Apr 23, 2017

Uh oh!

coveralls commented Apr 23, 2017

Uh oh!

aaltay commented Apr 24, 2017

Uh oh!

mingmxu commented Apr 25, 2017

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!

coveralls commented May 2, 2017

Uh oh!

coveralls commented May 2, 2017

Uh oh!

mingmxu commented May 2, 2017

Uh oh!

coveralls commented May 2, 2017

Uh oh!

mingmxu commented May 2, 2017

Uh oh!

davorbonaci 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!

coveralls commented May 4, 2017

Uh oh!

mingmxu commented May 4, 2017

Uh oh!

mingmxu commented May 4, 2017

Uh oh!

coveralls commented May 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants