[BEAM-8307] NPE in Calcite dialect when input PCollection has logical… #11581

rahul8383 · 2020-04-30T18:55:45Z

… type in schema, from JdbcIO Transform

When SqlTransform.query() PTransform is used with JdbcIO, where JdbcIO has logical types, an NPE is thrown in BeamSql when converting Beam Schema to Calcite RelDataType.

This PR adds standard logical types with URNs in schemas.logicaltypes package and adds Calcite's RelDataType Mapping for these logical types.

JdbcIO is modified to use these standard logical types to convert JDBC Schema to Beam Schema.

Standard logical types for DATE, TIME, TIME_WITH_TIMEZONE, TIMESTAMP_WITH_TIMEZONE are yet to be handled.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) 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.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

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

rahul8383 · 2020-04-30T19:13:42Z

R: @reuvenlax @amaliujia

aaltay · 2020-05-01T02:33:47Z

retest this please

reuvenlax · 2020-05-01T06:51:13Z

I'm wondering if this is a scalable solution.

In general SQL is supposed to be able to handle unknown logical types, and simply treat them as the base type. If you'r seeing a NPE, maybe we need to fix that. Where do you see the NPE?

rahul8383 · 2020-05-01T07:03:23Z

@reuvenlax
I have provided the source table schema and attached the NPE that I have faced in BEAM-8307

NPE is thrown as Calcite's RelDataType cannot be found for the JdbcIO Logical Type.

rahul8383 · 2020-05-01T08:57:24Z

I agree that this is not a scalable solution. Providing a Calcite RelDataType Mapping for every Logical Type defined(which is the solution presented in this PR) by every IO is not scalable.

Another approach to solving the problem is:
Provide Calcite RelDataType Mapping depending on the Base Type defined in the logical type.
But, as CHAR and VARCHAR logical types have Base Type as STRING, we have to choose a default Calcite RelDataType Mapping. The same thing applies to BINARY and VARBINARY logical types.
We also have to choose a default RelDataType Mapping if the Base Type is DATETIME.
By using this approach, I think we might be missing some built-in Aggregation functions provided by Calcite for specific types.

I was also thinking if we can use the IDENTIFIER of the logical type to determine the corresponding Calcite RelDataType. But, as the IDENTIFIER type is String and not an enum, it cannot be used. For example, all the logical types defined by JdbcIO use java.sql.JDBCType name as the IDENTIFIER.

Please correct me if my understanding is incorrect.

reuvenlax · 2020-05-02T01:34:31Z

Where do you see the NPE?

rahul8383 · 2020-05-02T01:58:15Z

JdbcIO.Read -> SqlTransform.query(SELECT COUNT(*) FROM PCOLLECTION /Any query/ ) throws NPE if the input PCollection to SqlTransform has JdbcIO specific Logical Types(defined in org.apache.beam.sdk.io.jdbc.LogicalTypes) in its Schema.

Please find the Source Table Schema and the attached exception stack trace in the JIRA ticket: BEAM-8307.

rahul8383 · 2020-05-02T03:28:15Z

I will make the necessary changes as suggested by @TheNeuralBit in https://lists.apache.org/thread.html/r281e2913379c9733f6ac5baa08f361cc4ebe880a9880b2d54d6095b0%40%3Cdev.beam.apache.org%3E

amaliujia · 2020-05-05T23:56:02Z

retest this please

rahul8383 · 2020-05-09T09:50:11Z

R: @TheNeuralBit

I found a bug while implementing this feature and raised PR #11609 to fix the bug as I thought that the bug fix could be cherry-picked in 2.21.0 release.

iemejia · 2020-05-12T08:24:56Z

retest this please

rahul8383 · 2020-05-12T09:33:50Z

The failing test org.apache.beam.runners.fnexecution.control.RemoteExecutionTest.testSplit should be unrelated to this change.

TheNeuralBit · 2020-05-12T15:54:41Z

Run SQL PostCommit

TheNeuralBit

Thanks @rahul8383! this is looking really good. I just have an ask around testing the logical types in SchemaCoder/RowCoder.

cc: @apilloud @robinyqiu for SQL type changes

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/LogicalDecimal.java

TheNeuralBit · 2020-05-12T16:07:58Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedBytes.java

-    } else {
-      return Arrays.copyOf(base, byteArraySize);
-    }
-  }


FYI @reuvenlax this PR is removing the ability to convert smaller byte arrays. As noted in #11609 (comment) it seems this logic is inaccessible anyway.

TheNeuralBit · 2020-05-12T16:19:14Z

Run SQL PostCommit

rahul8383 · 2020-05-12T16:57:53Z

@apilloud @amaliujia @robinyqiu
JdbcIO was using logical types which were specifically defined in JdbcIO package. When SqlTransform.query() was used with JdbcIO, an NPE is thrown as the Calcite's RelDataType Mapping could not be found for the logical types.
As all of these are standard logical types, this PR moved these logical types from JdbcIO package to schema.logicaltypes package so that other IOs which need these logical types can make use of it.
If BeamSql is supported for these standard logical types, BeamSql can be used with all the IOs that use these types.

To close this bug, some functional tests need to be added which uses the standard logical types defined in schema.logicaltypes package. Can you please provide some pointers as to what type of queries can be used to test these types?
Also CalciteUtils.CALCITE_TO_BEAM_TYPE_MAPPING doesn't specify these logical types. Does this mean the conversion of types is lossy? ex: will we lose the information about the maximum length of the String if VariableLengthString logical type is used in BeamSql?

apilloud · 2020-05-12T17:32:50Z

Beam SQL should be rejecting unknown logical types. We expect users to put their data into supported logical types before passing it in. This conversion is up to the user to implement and will likely be lossy. It would be significant work to make it do something different.

TheNeuralBit · 2020-05-12T18:46:19Z

Beam SQL should be rejecting unknown logical types.

The types that @rahul8383 is making into standard logical types here are well known SQL types. I don't think Beam SQL should consider them unknown.

… type in schema, from JdbcIO Transform

TheNeuralBit

Sorry for letting this languish @rahul8383 I got busy with the 2.22.0 release and neglected it :(

I have a few more comments now

TheNeuralBit · 2020-06-18T18:34:39Z

...c/main/java/org/apache/beam/sdk/schemas/logicaltypes/IdenticalBaseTAndInputTLogicalType.java

+
+/** A base class for LogicalTypes that use the same input type as the underlying base type. */
+@Experimental(Experimental.Kind.SCHEMAS)
+public abstract class IdenticalBaseTAndInputTLogicalType<T> implements Schema.LogicalType<T, T> {


Can you make this package-private?

TheNeuralBit · 2020-06-18T18:53:04Z

...nsions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtilsTest.java

+            .addField("variableBytes", Schema.FieldType.logicalType(VariableLengthBytes.of(100)))
+            .addField("fixedString", Schema.FieldType.logicalType(FixedLengthString.of(10)))
+            .addField("variableString", Schema.FieldType.logicalType(VariableLengthString.of(100)))
+            .addField("customDecimal", Schema.FieldType.logicalType(LogicalDecimal.of(10, 5)))


Could you add tests that use some or all of these new types in a SQL query? Right now I think this is the only test that exercises the new code in CalciteUtils, and it's not actually verifying the types will work in a SQL statement.

TheNeuralBit · 2020-06-18T18:54:16Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedLengthString.java

+    checkArgument(base == null || base.length() == length);
+    return base;
+  }
+}


Could you add constants for these new logical types in SqlTypes?

cc: @robinyqiu

TheNeuralBit · 2020-06-18T18:56:11Z

...c/main/java/org/apache/beam/sdk/schemas/logicaltypes/IdenticalBaseTAndInputTLogicalType.java

+
+/** A base class for LogicalTypes that use the same input type as the underlying base type. */
+@Experimental(Experimental.Kind.SCHEMAS)
+public abstract class IdenticalBaseTAndInputTLogicalType<T> implements Schema.LogicalType<T, T> {


Could you make this package-private? I think its only used in schemas.logicaltypes right now

TheNeuralBit · 2020-06-18T18:56:32Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/LogicalDecimal.java

+
+/** A LogicalType representing a Decimal type with custom precision and scale. */
+@Experimental(Experimental.Kind.SCHEMAS)
+public class LogicalDecimal extends IdenticalBaseTAndInputTLogicalType<BigDecimal> {


Can we just call this Decimal?

stale · 2020-08-22T19:25:29Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

stale · 2020-08-29T20:27:41Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

probot-autolabeler bot added build extensions io java jdbc sql labels Apr 30, 2020

rahul8383 force-pushed the using_BeamSql_with_JdbcIO branch from 0116e3a to 67c0621 Compare April 30, 2020 19:04

rahul8383 force-pushed the using_BeamSql_with_JdbcIO branch from 67c0621 to da13a84 Compare May 1, 2020 06:05

TheNeuralBit mentioned this pull request May 4, 2020

[BEAM-9840] Support for Parameterized Types when converting from HCat… #11569

Merged

4 tasks

rahul8383 mentioned this pull request May 5, 2020

[BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input #11609

Merged

4 tasks

iemejia requested a review from amaliujia May 6, 2020 23:30

rahul8383 force-pushed the using_BeamSql_with_JdbcIO branch from da13a84 to 8f5c293 Compare May 9, 2020 09:37

rahul8383 force-pushed the using_BeamSql_with_JdbcIO branch 2 times, most recently from b9d9353 to 7098248 Compare May 12, 2020 01:55

TheNeuralBit reviewed May 12, 2020

View reviewed changes

[BEAM-8307] NPE in Calcite dialect when input PCollection has logical…

4d222c5

… type in schema, from JdbcIO Transform

rahul8383 force-pushed the using_BeamSql_with_JdbcIO branch from 7098248 to 384c7fe Compare May 14, 2020 03:47

Add Test for Logcial Types which exercises coder

87e4ebc

rahul8383 force-pushed the using_BeamSql_with_JdbcIO branch from 384c7fe to 87e4ebc Compare May 14, 2020 04:05

TheNeuralBit self-requested a review May 21, 2020 22:10

TheNeuralBit reviewed Jun 18, 2020

View reviewed changes

stale bot added the stale label Aug 22, 2020

stale bot closed this Aug 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-8307] NPE in Calcite dialect when input PCollection has logical… #11581

[BEAM-8307] NPE in Calcite dialect when input PCollection has logical… #11581

rahul8383 commented Apr 30, 2020 •

edited

rahul8383 commented Apr 30, 2020 •

edited by iemejia

aaltay commented May 1, 2020

reuvenlax commented May 1, 2020

rahul8383 commented May 1, 2020

rahul8383 commented May 1, 2020

reuvenlax commented May 2, 2020

rahul8383 commented May 2, 2020 •

edited

rahul8383 commented May 2, 2020

amaliujia commented May 5, 2020

rahul8383 commented May 9, 2020

iemejia commented May 12, 2020

rahul8383 commented May 12, 2020

TheNeuralBit commented May 12, 2020

TheNeuralBit left a comment

TheNeuralBit May 12, 2020

TheNeuralBit commented May 12, 2020

rahul8383 commented May 12, 2020

apilloud commented May 12, 2020

TheNeuralBit commented May 12, 2020

TheNeuralBit left a comment

TheNeuralBit Jun 18, 2020

TheNeuralBit Jun 18, 2020

TheNeuralBit Jun 18, 2020

TheNeuralBit Jun 18, 2020

TheNeuralBit Jun 18, 2020

stale bot commented Aug 22, 2020

stale bot commented Aug 29, 2020

[BEAM-8307] NPE in Calcite dialect when input PCollection has logical… #11581

[BEAM-8307] NPE in Calcite dialect when input PCollection has logical… #11581

Conversation

rahul8383 commented Apr 30, 2020 • edited

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

rahul8383 commented Apr 30, 2020 • edited by iemejia

aaltay commented May 1, 2020

reuvenlax commented May 1, 2020

rahul8383 commented May 1, 2020

rahul8383 commented May 1, 2020

reuvenlax commented May 2, 2020

rahul8383 commented May 2, 2020 • edited

rahul8383 commented May 2, 2020

amaliujia commented May 5, 2020

rahul8383 commented May 9, 2020

iemejia commented May 12, 2020

rahul8383 commented May 12, 2020

TheNeuralBit commented May 12, 2020

TheNeuralBit left a comment

Choose a reason for hiding this comment

TheNeuralBit May 12, 2020

Choose a reason for hiding this comment

TheNeuralBit commented May 12, 2020

rahul8383 commented May 12, 2020

apilloud commented May 12, 2020

TheNeuralBit commented May 12, 2020

TheNeuralBit left a comment

Choose a reason for hiding this comment

TheNeuralBit Jun 18, 2020

Choose a reason for hiding this comment

TheNeuralBit Jun 18, 2020

Choose a reason for hiding this comment

TheNeuralBit Jun 18, 2020

Choose a reason for hiding this comment

TheNeuralBit Jun 18, 2020

Choose a reason for hiding this comment

TheNeuralBit Jun 18, 2020

Choose a reason for hiding this comment

stale bot commented Aug 22, 2020

stale bot commented Aug 29, 2020

rahul8383 commented Apr 30, 2020 •

edited

rahul8383 commented Apr 30, 2020 •

edited by iemejia

rahul8383 commented May 2, 2020 •

edited