After thinking about this more I've softened on it some, but I'm still a little wary. I like Kenn's suggestion:
> - it is OK to convert known logical types like MillisInstant or NanosInstant to a ZetaSQL "TIMESTAMP" or Calcite SQL "TIMESTAMP WITH LOCAL TIME ZONE" > - it is OK to convert unknown logical types into their representation type > - coming out the other side of a SQL statement, a user should expect the type to be a row where every field has a type from the SQL dialect; the input types are gone I think what you're proposing is basically a preprocessing step on SqlTransform that demotes any unrecognized logical types to their representation. Is that a fair characterization? I wonder if we could push the demotion further into the transform to enable some additional uses. For example "SELECT * FROM PCOLL WHERE ..." - if some logical type SQL knows nothing about is pulled into the SELECT *, it seems reasonable (from a user-perspective) to just pass the type through untouched, and automatically demoting it might be surprising. What if we had the ability to implicitly convert logical types to their representation? Then the above example would work, because no implicit conversion is necessary, and also relations that introspect the logical type (e.g. projecting f_location.lat) could work by including an implicit conversion. The implicit conversion would still be a one-way operation, but it would only happen when necessary. I really have no idea how complicated this would be to implement. Would it be feasible? Is it ill-advised? Brian On Fri, May 15, 2020 at 2:43 PM Reuven Lax <re...@google.com> wrote: > I would not describe the base type as the "wire type." If that were true, > then the only base type we should support should be byte array. > > My simple point is that this is no different than normal schema fields. > You will find many normal schemas containing data encoded into other field > types. You will also find this in SQL databases. If naming these fields > with a logical type causes us to throw up our hands and declare that they > can't be introspectable, then our users will likely just avoid logical > types and encode this data into their own LONG and STRING fields. It will > also mean that SQL will not work well over Avro or Proto input. > Is the Avro/Proto connection really a problem? I would think that we could map most common Avro or Proto types to either primitive types or "standard" logical types which are the same as the ones used by SQL wherever possible, e.g. NanosInstant and NanosDuration added for proto. > > Reuven > > On Fri, May 15, 2020 at 2:19 PM Andrew Pilloud <apill...@google.com> > wrote: > >> My understanding is that the base type is effectively the wire format at >> the type, where the logical type is the in-memory representation for Java. >> For org.joda.time.Instant, this is just a wrapper around the underlying >> Long. However for the Date logical type, the LocalDate type has struct as >> the in-memory representation. There is a performance penalty to this >> conversion and I think there are cases where users (SQL for example) would >> want a fast path that skips the serialization/deserialization steps if >> possible. >> >> As for more complex logical types, I would urge caution in making >> assumptions around the base types. I expect there will be base types of >> strings containing JSON or byte arrays containing protobuf or avro. ZetaSQL >> has a civil time module that packs a struct into a Long. All of these are >> things you can write SQL around, but I think the users might be somewhat >> surprised when they come out less than usable by default. >> >> On Fri, May 15, 2020 at 2:12 PM Reuven Lax <re...@google.com> wrote: >> >>> >>> >>> On Fri, Apr 24, 2020 at 11:56 AM Brian Hulette <bhule...@google.com> >>> wrote: >>> >>>> When we created the portable representation of schemas last summer we >>>> intentionally did not include DATETIME as a primitive type [1], even though >>>> it already existed as a primitive type in Java [2]. There seemed to be >>>> consensus around a couple of points: 1) At the very least DATETIME is a >>>> poor name and it should be renamed to INSTANT or TIMESTAMP, and 2) a >>>> logical type is better suited for this purpose. >>>> >>>> Since then, it's been my intention to replace Java's DATETIME with a >>>> MillisInstant logical type backed by Long milliseconds since the epoch (see >>>> BEAM-7554 [3]). I've finally managed to get a PR together that does so (and >>>> keeps tests passing): https://github.com/apache/beam/pull/11456 >>>> >>>> There could be a couple of concerns with this PR that I think would be >>>> better discussed here rather than on github. >>>> >>>> >>>> ## Breaking changes in low-level APIs >>>> The PR includes some breaking changes in public, low-level schema APIs: >>>> It removes DATETIME from the TypeName enum [4], and it will also change the >>>> type returned by Row#getBaseValue for DATETIME/MillisInstant fields (Long >>>> milliseconds since epoch rather than org.joda.time.Instant). Both of these >>>> changes have the potential to break user code. That being said I think the >>>> risk is justified for a few reasons: >>>> - These are lower-level APIs that we don't intend for most users to >>>> use. The preferred higher-level APIs (SQL, Schema transforms, and inferred >>>> schemas for user types) should seamlessly transition over to the new >>>> MillisInstant logical type. >>>> - Schemas are an experimental feature. >>>> - We can clearly document this breaking change in the release that >>>> includes it. >>>> >>> >>> I am a bit worried about this. Schemas have been marked experimental >>> for. almost two years, and I've found that many Beam users are using them. >>> If we make a breaking change, can we make sure that it is extremely clear >>> to users how to fix their code? I'm afraid we are in the situation now >>> where the code is theoretically experimental but practically is widely used. >>> >>> >>> >>>> >>>> >>>> ## Mixing joda and java 8 time >>>> The NanosInstant logical type that Alex added uses java.time.Instant as >>>> it's input type, while my MillisInstant type uses org.joda.time.Instant for >>>> compatibility with the rest of Beam and the previous DATETIME primitive >>>> type. It feels weird, but it also makes a certain sort of sense to use joda >>>> time (which has millisecond precision) for MillisInstant, and java 8 time >>>> (which has nanos) for NanosInstant. Also, the choice shouldn't have a >>>> significant effect on end users - the schema inference code could generate >>>> conversions between java 8 time and joda time (as we already do for >>>> converting between various joda time types [5]) so user types can use >>>> either one. >>>> >>> >>> +1 >>> >>> >>>> >>>> >>>> ## Arbitrary Logical Types and SQL >>>> Previously much of the SQL code was written to operate on the _base >>>> type_ value for any logical types. So for the new MillisInstant type, SQL >>>> would attempt to operate on the underlying Long, rather than on a >>>> org.joda.time.Instant instance. Thus when I switched over to a >>>> MillisInstant logical type as the default for SQL date and time types any >>>> tests that used them began failing with ClassCastExceptions. >>>> >>>> My solution was just to update SQL code to only ever reference the >>>> input type (i.e. org.joda.time.Instant) for logical types. A good example >>>> of this type of change is in BeamAggregationRel [6]. >>>> >>>> My change does pass all of the SQL tests (including internal Google >>>> tests), but I'm a little concerned that using the base type throughout SQL >>>> was a conscious decision that I'm stomping on. In theory, it could ensure >>>> that SqlTransform would be able to operate on user types that contain >>>> custom user logical types, without requiring SqlTransform to understand >>>> those types. This is just speculation though, there aren't actually any >>>> tests verifying that functionality. Also, I don't think any such tests >>>> would have worked since there are several places where we explicitly check >>>> for particular logical types (e.g. in BeamCalcRel [7]). >>>> >>>> Personally I'm ok with this move, because 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. >>>> >>> >>> I disagree with this personally: >>> >>> Arguably, _every_ schema has "additional meaning" beyond the fields, and >>> a LogicalType is just a nice way of naming that and allowing the use of >>> other classes to manage that type. For example, Let's say you have a record >>> containing latitude and longitude fields - today you can easily write SQL >>> over these fields. Later you decide that this is a very common pattern in >>> your data and decide to create a Location logical type; this also lets you >>> represent this with a convenience Location class that has many other >>> helpful features (distance functions, builders, etc.). It seems unfortunate >>> if you suddenly lose the ability to easily run SQL over this type. >>> Introducing a logical type hasn't changed the semantic meaning of your data >>> at. all. All it has done is let you name it, and it would be unfortunate if >>> analyzing your data stream suddenly became harder as a result. >>> >> I would argue that defining the logical type *has* changed the semantic meaning. Before you just had a tuple of floating point numbers that happened to have fields named lat and long. After defining a logical type you have a location, and lat and long have a restricted range. >>> Another reason I don't like this, is that the pattern that we have >>> encouraged (and already use extensively) is to use logical types when >>> converting from external formats such as avro, proto, kryo, etc. If >>> logical types aren't supported by SQL, then we are effectively saying that >>> data coming from these formats can't be easily analyzed with SQL (or SQL >>> will have to special case the logical types used by every single such input >>> format, which does not seem sustainable). >>> >> >>> I think the problem you ran into is that today an aggregation deals with >>> logical types in an all-or-nothing manner. You either aggregate everything >>> by the representation type or everything by the logical type. If we instead >>> allowed SQL to specify well-known logical types (e.g. Instant), I think >>> that this problem would go away. >>> >>> >>>> >>>> >>>> If anyone has any concerns with these points could you raise them here? >>>> Otherwise I'd like to move ahead with the PR. >>>> (Sorry this message turned out to be a bit of a novel. LMK if it would >>>> be better served by separate threads, or maybe a doc.) >>>> >>>> Thanks! >>>> Brian >>>> >>>> [1] >>>> https://github.com/apache/beam/blob/ef267d97a7cc6bb1bece30af940ee9d8e487f963/model/pipeline/src/main/proto/schema.proto#L58 >>>> [2] >>>> https://github.com/apache/beam/blob/ef267d97a7cc6bb1bece30af940ee9d8e487f963/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java#L682 >>>> [3] https://issues.apache.org/jira/browse/BEAM-7554 >>>> [4] >>>> https://github.com/apache/beam/blob/ef267d97a7cc6bb1bece30af940ee9d8e487f963/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java#L422 >>>> [5] >>>> https://github.com/apache/beam/blob/ef267d97a7cc6bb1bece30af940ee9d8e487f963/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ByteBuddyUtils.java#L1155-L1158 >>>> [6] >>>> https://github.com/apache/beam/pull/11456/files#diff-98d3c841460c6d59f281c40040260f2d >>>> [7] >>>> https://github.com/apache/beam/blob/ef267d97a7cc6bb1bece30af940ee9d8e487f963/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java#L409 >>>> >>>
