You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/08/04 03:19:07 UTC
[GitHub] [beam] amaliujia opened a new pull request #12461: [BEAM-10633] UdfImpl should support return java.util.List
amaliujia opened a new pull request #12461:
URL: https://github.com/apache/beam/pull/12461
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] 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.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12461: [BEAM-10633] UdfImpl hits NPE when returns a List
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r466619308
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
##########
@@ -101,6 +104,21 @@ public void testJodaTimeUdfUdaf() throws Exception {
pipeline.run().waitUntilFinish();
}
+ @Test
+ public void testListUdf() throws Exception {
Review comment:
I'm fine with doing this in a follow up PR, since the other tests in this class follow the same pattern.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12461: [BEAM-10633] UdfImpl should be able to return java.util.List
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r465341578
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java
##########
@@ -285,16 +286,23 @@ private static RelDataType toRelDataType(
* SQL-Java type mapping, with specified Beam rules: <br>
* 1. redirect {@link AbstractInstant} to {@link Date} so Calcite can recognize it.
Review comment:
I updated the List case. The ByteString case wasn't added by me so I don't have context around it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12461: [BEAM-10633] UdfImpl hits NPE when returns a List
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r465400167
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
##########
@@ -101,6 +104,21 @@ public void testJodaTimeUdfUdaf() throws Exception {
pipeline.run().waitUntilFinish();
}
+ @Test
+ public void testListUdf() throws Exception {
Review comment:
We should separate this into two modular test cases, one with a UDF that takes an array as an argument (e.g. the length function), and one with a UDF that returns an array (e.g. array constructor, like your previous test).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia merged pull request #12461: [BEAM-10633] UdfImpl hits NPE when returns a List
Posted by GitBox <gi...@apache.org>.
amaliujia merged pull request #12461:
URL: https://github.com/apache/beam/pull/12461
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #12461: [BEAM-10633] UdfImpl hits NPE when returns a List
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #12461:
URL: https://github.com/apache/beam/pull/12461#issuecomment-670133998
+1 on refactoring it in another PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #12461: [BEAM-10633] UdfImpl hits NPE when returns a List
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #12461:
URL: https://github.com/apache/beam/pull/12461#issuecomment-668847499
Also appends a new commit to extend the test case to cover when arguments are also java.util.List.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12461: [BEAM-10633] UdfImpl should be able to return java.util.List
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r465289699
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
##########
@@ -101,6 +103,21 @@ public void testJodaTimeUdfUdaf() throws Exception {
pipeline.run().waitUntilFinish();
}
+ @Test
+ public void testListUdf() throws Exception {
+ Schema resultType = Schema.builder().addArrayField("array_field", FieldType.INT64).build();
+
+ Row row1 = Row.withSchema(resultType).addValue(Arrays.asList(1L)).build();
Review comment:
Nit: why are row1, sql1, etc. variable names numbered when there's only one of each?
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java
##########
@@ -285,16 +286,23 @@ private static RelDataType toRelDataType(
* SQL-Java type mapping, with specified Beam rules: <br>
* 1. redirect {@link AbstractInstant} to {@link Date} so Calcite can recognize it.
*
- * @param rawType
+ * @param type
* @return
*/
- public static RelDataType sqlTypeWithAutoCast(RelDataTypeFactory typeFactory, Type rawType) {
+ public static RelDataType sqlTypeWithAutoCast(RelDataTypeFactory typeFactory, Type type) {
// For Joda time types, return SQL type for java.util.Date.
- if (rawType instanceof Class && AbstractInstant.class.isAssignableFrom((Class<?>) rawType)) {
+ if (type instanceof Class && AbstractInstant.class.isAssignableFrom((Class<?>) type)) {
return typeFactory.createJavaType(Date.class);
- } else if (rawType instanceof Class && ByteString.class.isAssignableFrom((Class<?>) rawType)) {
+ } else if (type instanceof Class && ByteString.class.isAssignableFrom((Class<?>) type)) {
return typeFactory.createJavaType(byte[].class);
+ } else if (type instanceof ParameterizedType
+ && java.util.List.class.isAssignableFrom(
+ (Class<?>) ((ParameterizedType) type).getRawType())) {
+ ParameterizedType parameterizedType = (ParameterizedType) type;
+ Class<?> genericType = (Class<?>) parameterizedType.getActualTypeArguments()[0];
+ RelDataType collectionElementType = typeFactory.createJavaType(genericType);
+ return typeFactory.createArrayType(collectionElementType, -1L);
Review comment:
```suggestion
return typeFactory.createArrayType(collectionElementType, UNLIMITED_ARRAY_SIZE);
```
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java
##########
@@ -285,16 +286,23 @@ private static RelDataType toRelDataType(
* SQL-Java type mapping, with specified Beam rules: <br>
* 1. redirect {@link AbstractInstant} to {@link Date} so Calcite can recognize it.
Review comment:
We should probably update this comment to explain how/why ByteString and List are special cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12461: [BEAM-10633] UdfImpl hits NPE when returns a List
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r466615046
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
##########
@@ -101,6 +104,21 @@ public void testJodaTimeUdfUdaf() throws Exception {
pipeline.run().waitUntilFinish();
}
+ @Test
+ public void testListUdf() throws Exception {
Review comment:
The tests LGTM, but we should separate the two blocks into separate `@Test` methods. That way we can easily tell if one case fails but the other succeeds. And it keeps the tests smaller and easier to read.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12461: [BEAM-10633] UdfImpl hits NPE when returns a List
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r465449987
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
##########
@@ -101,6 +104,21 @@ public void testJodaTimeUdfUdaf() throws Exception {
pipeline.run().waitUntilFinish();
}
+ @Test
+ public void testListUdf() throws Exception {
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12461: [BEAM-10633] UdfImpl should be able to return java.util.List
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r465341147
##########
File path: sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
##########
@@ -101,6 +103,21 @@ public void testJodaTimeUdfUdaf() throws Exception {
pipeline.run().waitUntilFinish();
}
+ @Test
+ public void testListUdf() throws Exception {
+ Schema resultType = Schema.builder().addArrayField("array_field", FieldType.INT64).build();
+
+ Row row1 = Row.withSchema(resultType).addValue(Arrays.asList(1L)).build();
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12461: [BEAM-10633] UdfImpl should be able to return java.util.List
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12461:
URL: https://github.com/apache/beam/pull/12461#discussion_r465341297
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java
##########
@@ -285,16 +286,23 @@ private static RelDataType toRelDataType(
* SQL-Java type mapping, with specified Beam rules: <br>
* 1. redirect {@link AbstractInstant} to {@link Date} so Calcite can recognize it.
*
- * @param rawType
+ * @param type
* @return
*/
- public static RelDataType sqlTypeWithAutoCast(RelDataTypeFactory typeFactory, Type rawType) {
+ public static RelDataType sqlTypeWithAutoCast(RelDataTypeFactory typeFactory, Type type) {
// For Joda time types, return SQL type for java.util.Date.
- if (rawType instanceof Class && AbstractInstant.class.isAssignableFrom((Class<?>) rawType)) {
+ if (type instanceof Class && AbstractInstant.class.isAssignableFrom((Class<?>) type)) {
return typeFactory.createJavaType(Date.class);
- } else if (rawType instanceof Class && ByteString.class.isAssignableFrom((Class<?>) rawType)) {
+ } else if (type instanceof Class && ByteString.class.isAssignableFrom((Class<?>) type)) {
return typeFactory.createJavaType(byte[].class);
+ } else if (type instanceof ParameterizedType
+ && java.util.List.class.isAssignableFrom(
+ (Class<?>) ((ParameterizedType) type).getRawType())) {
+ ParameterizedType parameterizedType = (ParameterizedType) type;
+ Class<?> genericType = (Class<?>) parameterizedType.getActualTypeArguments()[0];
+ RelDataType collectionElementType = typeFactory.createJavaType(genericType);
+ return typeFactory.createArrayType(collectionElementType, -1L);
Review comment:
Nice catch! Made the suggested change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #12461: [BEAM-10633] UdfImpl should support return java.util.List
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #12461:
URL: https://github.com/apache/beam/pull/12461#issuecomment-668356948
R: @ibzib
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org