[BEAM-2520] add UDF/UDAF in BeamSql.query/simpleQuery #3491

mingmxu · 2017-07-03T05:25:36Z

No description provided.

mingmxu · 2017-07-04T18:26:02Z

R: @xumingming @lukecwik
CC: @takidau @jbonofre

coveralls · 2017-07-04T19:45:42Z

Changes Unknown when pulling 76cce07 on XuMingmin:BEAM-2520 into ** on apache:DSL_SQL**.

xumingming · 2017-07-05T02:24:18Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/BeamSql.java

-    return new QueryTransform(sqlQuery);
-
+  public static QueryTransform query(String sqlQuery) {
+    return new AutoValue_BeamSql_QueryTransform.Builder()


new AutoValue_BeamSql_QueryTransform.Builder() => new QueryTransform.builder()?

xumingming · 2017-07-05T02:24:51Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/BeamSql.java

-  simpleQuery(String sqlQuery) throws Exception {
-    return new SimpleQueryTransform(sqlQuery);
+  public static SimpleQueryTransform simpleQuery(String sqlQuery) throws Exception {
+    return new AutoValue_BeamSql_SimpleQueryTransform.Builder()


new AutoValue_BeamSql_SimpleQueryTransform.Builder() => SimpleQueryTransform.builder()?

xumingming

LGTM (except two small issues.)

mingmxu · 2017-07-05T05:08:29Z

Good point, however seems the method of builder()/toBuiler() was not accurate.

xumingming · 2017-07-05T05:49:09Z

Seems different from auto-value's usage convention: https://github.com/google/auto/blob/master/value/userguide/builders.md

xumingming · 2017-07-05T05:50:23Z

I quote:

public void testAnimal() {
  Animal dog = Animal.builder().setName("dog").setNumberOfLegs(4).build();
  assertEquals("dog", dog.name());
  assertEquals(4, dog.numberOfLegs());

  // You probably don't need to write assertions like these; just illustrating.
  assertTrue(
      Animal.builder().setName("dog").setNumberOfLegs(4).build().equals(dog));
  assertFalse(
      Animal.builder().setName("cat").setNumberOfLegs(4).build().equals(dog));
  assertFalse(
      Animal.builder().setName("dog").setNumberOfLegs(2).build().equals(dog));

  assertEquals("Animal{name=dog, numberOfLegs=4}", dog.toString());
}

mingmxu · 2017-07-05T06:14:17Z

--update withUdf/withUdaf methods;

interesting, the code of TextIO/KafkaIO is different from the example. I suppose abstract Builder toBuilder() is better here, to avoid return this in function withUdf/withUdaf.

@lukecwik any comments?

xumingming · 2017-07-05T06:20:36Z

Indeed interesting :)

coveralls · 2017-07-05T07:28:31Z

Changes Unknown when pulling 2455dc4 on XuMingmin:BEAM-2520 into ** on apache:DSL_SQL**.

mingmxu · 2017-07-10T17:04:53Z

@takidau any suggestion which one is prefered in AutoValue?

static Builder builder() {
  return new AutoValue_BeamSql_QueryTransform.Builder();
}

OR

abstract Builder toBuilder();

takidau · 2017-07-12T06:29:22Z

Re builder vs toBuilder, perhaps I'm misunderstanding the question, but they serve different purposes, don't they? I do prefer defining a static builder() method to directly invoking the generated AutoValue_BeamSql_SimpleQueryTransform.Builder() constructor (thought we seem to have both patterns in the Beam codebase currently).

takidau · 2017-07-12T06:34:19Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/BeamSql.java

      return PCollectionTuple.of(new TupleTag<BeamSqlRow>(PCOLLECTION_TABLE_NAME), input)
-          .apply(new QueryTransform(sqlQuery, sqlEnv));
+          .apply(new AutoValue_BeamSql_QueryTransform.Builder()


Should this be reduced to .apply(toBuilder().build()) as well?

Ping on this comment, since it wasn't addressed or answered. Can't this be simplified?

missed this line, just update.

mingmxu · 2017-07-12T07:33:40Z

Thanks @takidau for your advise, updated builder and rebase to latest repository.

coveralls · 2017-07-12T08:46:17Z

Changes Unknown when pulling 2284cde on XuMingmin:BEAM-2520 into ** on apache:DSL_SQL**.

takidau · 2017-07-12T18:10:22Z

dsls/sql/src/main/java/org/apache/beam/dsls/sql/BeamSql.java

+    /**
+     * register a UDF function used in this query.
+     */
+     public QueryTransform withUdf(String functionName, Class<?> clazz, String methodName){


Is it necessary to support having arbitrarily named (and potentially multiple) UDF methods on a single class? Why not just have a simply BeamSqlUdf interface you must implement? And why not accept a lambda?

Is it necessary to support having arbitrarily named (and potentially multiple) UDF methods on a single class?
[Mingmin]: I would like to keep the capacity to put similar or related methods together.

Why not just have a simply BeamSqlUdf interface you must implement?
[Mingmin]: That's how we do with UDAF, UDF is a little different as it may have variable, even optional parameters. Annotation (like DoFn) sounds a solution but the existing definition is more simple IMO.

And why not accept a lambda?
[Mingmin]: I remember Beam is still on JDK 7, --sorry that I may miss something.

Okay

We may want to revisit this when we do a holistic API review before merge to master, but fine for now then.

That's true, but for Java 8 users it'd be nice to have an overload supporting a lambda. Not necessary for this PR though.

takidau · 2017-07-12T20:09:17Z

Looks like there are build issues, can you please fix?

Also, regarding the change in SimpleQueryTransform.expand, my question was whether toBuilder().build() could be used in place of manually constructing a copy of this. It's both shorter, and more maintainable since the AutoValue generator will keep track of making sure new fields are also copied as the AutoValue object evolves over time.

update AutoValue builder() update with methods update Builder/build in AutoValue rebase simplify AutoValue

mingmxu · 2017-07-12T21:05:50Z

rebased to fix the compile issue.

toBuilder().build() doesn't work here, as SimpleQueryTransform.expand returns QueryTransform, they've different interfaces.

Regarding to the interface of UDF, your concern is reasonable, created BEAM-2609 for further discussion.

coveralls · 2017-07-12T22:16:14Z

Changes Unknown when pulling 2e0abdd on XuMingmin:BEAM-2520 into ** on apache:DSL_SQL**.

takidau · 2017-07-12T22:50:03Z

Thanks for the explanation on the toBuilder question, and for opening the Jira. Merging.

takidau · 2017-07-12T22:58:57Z

Merged, feel free to close.

mingmxu force-pushed the BEAM-2520 branch from f52c452 to 76cce07 Compare July 4, 2017 18:25

xumingming reviewed Jul 5, 2017

View reviewed changes

xumingming approved these changes Jul 5, 2017

View reviewed changes

mingmxu force-pushed the BEAM-2520 branch from 0eda209 to 2455dc4 Compare July 5, 2017 06:11

takidau suggested changes Jul 12, 2017

View reviewed changes

mingmxu force-pushed the BEAM-2520 branch from 2455dc4 to 2284cde Compare July 12, 2017 07:32

takidau suggested changes Jul 12, 2017

View reviewed changes

mingmxu added 2 commits July 12, 2017 13:38

support UDF/UDAF in BeamSql

26617c5

update AutoValue builder() update with methods update Builder/build in AutoValue rebase simplify AutoValue

rebase to fix compile issue

2e0abdd

mingmxu force-pushed the BEAM-2520 branch from c72d56c to 2e0abdd Compare July 12, 2017 20:59

asfgit pushed a commit that referenced this pull request Jul 12, 2017

[BEAM-2520] This closes #3491

d89d1ee

mingmxu closed this Jul 12, 2017

mingmxu deleted the BEAM-2520 branch July 12, 2017 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-2520] add UDF/UDAF in BeamSql.query/simpleQuery #3491

[BEAM-2520] add UDF/UDAF in BeamSql.query/simpleQuery #3491

mingmxu commented Jul 3, 2017

mingmxu commented Jul 4, 2017

coveralls commented Jul 4, 2017

xumingming Jul 5, 2017

xumingming Jul 5, 2017

xumingming left a comment

mingmxu commented Jul 5, 2017

xumingming commented Jul 5, 2017

xumingming commented Jul 5, 2017

mingmxu commented Jul 5, 2017

xumingming commented Jul 5, 2017

coveralls commented Jul 5, 2017

mingmxu commented Jul 10, 2017

takidau commented Jul 12, 2017

takidau Jul 12, 2017

takidau Jul 12, 2017

mingmxu Jul 12, 2017

mingmxu commented Jul 12, 2017

coveralls commented Jul 12, 2017

takidau Jul 12, 2017

mingmxu Jul 12, 2017

takidau Jul 12, 2017

takidau commented Jul 12, 2017

mingmxu commented Jul 12, 2017

coveralls commented Jul 12, 2017

takidau commented Jul 12, 2017

takidau commented Jul 12, 2017

[BEAM-2520] add UDF/UDAF in BeamSql.query/simpleQuery #3491

[BEAM-2520] add UDF/UDAF in BeamSql.query/simpleQuery #3491

Conversation

mingmxu commented Jul 3, 2017

mingmxu commented Jul 4, 2017

coveralls commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xumingming left a comment

Choose a reason for hiding this comment

mingmxu commented Jul 5, 2017

xumingming commented Jul 5, 2017

xumingming commented Jul 5, 2017

mingmxu commented Jul 5, 2017

xumingming commented Jul 5, 2017

coveralls commented Jul 5, 2017

mingmxu commented Jul 10, 2017

takidau commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingmxu commented Jul 12, 2017

coveralls commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takidau commented Jul 12, 2017

mingmxu commented Jul 12, 2017

coveralls commented Jul 12, 2017

takidau commented Jul 12, 2017

takidau commented Jul 12, 2017