Thanks for your suggestion, Brian. I will move the logical types explicitly defined for JdbcIO in org.apache.beam.sdk.io.jdbc.LogicalTypes to org.apache.beam.sdk.schemas.logicaltypes with a URN identifier. If any other IO defines logical types which correspond to SQL data types, all those logical types can be moved to org.apache.beam.sdk.schemas.logicaltypes or use the logical types which are already defined in this package, so that these IOs can be used with BeamSql.
On Sat, May 2, 2020 at 3:00 AM Brian Hulette <[email protected]> wrote: > > > On Thu, Apr 30, 2020 at 11:26 PM rahul patwari <[email protected]> > wrote: > >> Hi, >> >> A JIRA ticket is raised to track this bug: BEAM-8307 >> <https://issues.apache.org/jira/browse/BEAM-8307> >> >> I have raised a PR: https://github.com/apache/beam/pull/11581 to fix the >> issue. >> >> This PR takes care of using BeamSql with JdbcIO. >> I would be interested to contribute if any other IOs supported by Beam >> requires a similar fix like the one in this PR so that they can be used >> with BeamSql. >> >> What could be a cleaner approach, in general, to handle this for all the >> IOs? >> >> Also, what can be done to support BeamSql with User-Defined Logical >> Types? >> Should they be converted to one of the Beam SQL Types[1] before applying >> SqlTransform.query()? >> Should we expose an interface to provide Calcite RelDataType Mapping for >> User-Defined Logical Types? >> > > First I want to say I don't think the JdbcIO types you're adding fixes for > in https://github.com/apache/beam/pull/11581 should be considered > "user-defined logical types". These are all well known types that we > *should* have standard logical types for in schemas.logicaltypes, with a > URN identifier. Then both JDBC and SQL can reference those same logical > types and there's no need to add a mapping in CalciteUtils. This is what > +Robin > Qiu <[email protected]> is doing for DATE in [1]. > > As far as user-defined logical types in general, personally I don't think > SqlTransform should accept them. I brought this up at the end of a recent > message [2]. I'll copy the relevant part here so you don't have to wade > through it :) > > > ... I'm against the idea of > > implicitly stripping away logical types like this in schema-aware > > transforms. A logical type indicates that the value has some additional > > meaning beyond just the base type (and maybe a restricted range), and I > > don't think transforms should be able to ignore that meaning unless the > > user explicitly allows it, or first converts to the base type themselves. > > If users really want to process custom logical types with SQL, they could > just convert them to their base types first. If this is common, we could > add an option to SqlTransform to have it do this automatically for > unrecognized types. > > [1] https://github.com/apache/beam/pull/11272 > [2] > https://lists.apache.org/thread.html/r2e05355b74fb5b8149af78ade1e3539ec08371a9a4b2b9e45737e6be%40%3Cdev.beam.apache.org%3E > > > >> Let me know your thoughts. >> >> [1]: >> https://github.com/apache/beam/blob/b8aa8486f336df6fc9cf581f29040194edad3b87/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java#L43 >> >> Regards, >> Rahul >> >
