PARQUET-906: Add LogicalType annotation. #51

rdblue · 2017-03-24T23:32:35Z

This commit adds a LogicalType union and a field for this logical type to SchemaElement. Adding a new structure for logical types is needed for a few reasons:

Adding to the ConvertedType enum is not forward-compatible. Adding new types to the LogicalType union is forward-compatible.
Using a struct for each type allows additional metadata, like isAdjustedToUTC, without adding more fields to SchemaElement that don't apply to all types.
Types without additional metadata can be updated later. For example, adding an encoding field to StringType when it is needed.

rdblue · 2017-03-24T23:38:24Z

@julienledem, @mkornacker, here is an early version of additional metadata for time and timestamp types.

When I looked at adding just metadata for Time and Timestamp, the work was nearly identical to the LogicalType union that this PR includes, but required more spec to state what metadata should be associated with a certain ConvertedType. I think it is simpler to solve the compatibility problem (can't add to a thrift enum) and add the extra metadata at the same time by replacing ConvertedType with this new LogicalType.

I haven't updated all the docs yet because I wanted to get feedback on the approach first. Please comment. Thanks!

julienledem

I like the general idea for this.

julienledem · 2017-04-04T13:51:37Z

src/main/thrift/parquet.thrift

-   * Annotates a column that is always null
-   * Sometimes when discovering the schema of existing data
-   * values are always null
-   */


If you don't want to duplicate the comment, maybe refer to NullType below

Oops, I intended to remove NULL entirely because it is an unreleased type that can be replaced with the LogicalType version.

julienledem · 2017-04-04T13:53:35Z

src/main/thrift/parquet.thrift

+
+/** Timestamp logical type annotation */
+struct TimestampType {
+  1: required bool isAdjustedToUTC


How about "withoutTimeZone" meaning the opposite of "isAdjustedToUTC"

isAdjustedToUTC is more clear. With or without time zone is language from the SQL spec, which is very difficult to understand and apply. Rather than relying on it, this captures a very specific piece of information: whether the timestamp was adjusted to UTC from its original offset (or would have been if the original offset is UTC).

if isAdjusted to UTC is true, we need to record the original TZ. So we should add an optional String timezone field. (and then we don't need isAdjustedToUTC since it is implied by timezone != null )
Something similar similar to Arrow:
https://github.com/apache/arrow/blob/a4f29f3a3ff1c64a6f547bfb0d5e4500142ea5ec/format/Schema.fbs#L117

The model we discussed for Parquet is different from the Arrow spec.
In Arrow, both Timestamp and TimestampTz are stored in UTC. The writer Timezone string is additionally stored to re-compute the Timestamp values.
For Parquet, the idea is to store Timestamp values as is from epoch without conversion to UTC.
The conversion to UTC happens only for TimestampTz values.
The parquet approach is slightly efficient since we don't have to re-compute the Timestamp values thereby enabling bulkload of the column.
The presence of a writer Timezone in the file metadata will also prohibit concatenation of files from different Timezones.

In Arrow, both Timestamp and TimestampTz are stored in UTC

This isn't accurate, see: https://github.com/apache/arrow/blob/master/format/Schema.fbs#L120. In Arrow, if the timestamp metadata does not have a time zone, it is time zone naive, not UTC.

From naive timestamp values, we can choose later to localize to UTC (which is a no-op) or localize to some other time zone (which will adjust the values to the UTC values internally based on the tz database)

I opened https://issues.apache.org/jira/browse/ARROW-1020 to clarify in the comments in Schema.fbs

When timezone is set, the physical values are UTC timestamps regardless of the time zone of the writer or the client, so changing the timezone does not change the underlying integers.

I don't think we need to store the original zone if isAdjustedToUTC is true. This isn't done by other systems, and there is no guarantee that there is a single zone that the timestamps were converted from. This just indicates that whatever the source zone, the values have been normalized to the same one, UTC.

I linked the parquet and arrow jiras about timestamp types. There's a discution there about timestamp types and timezones:
https://issues.apache.org/jira/browse/PARQUET-905
https://issues.apache.org/jira/browse/ARROW-637

We spoke about this on the Arrow sync yesterday.

In Arrow, we have 3 cases:

No time zone in the metadata: the data is time zone naive. One can later localize to a time zone (which may alter the values, because they will be internally normalized to UTC)

Time zone in the metadata. Whether the time zone is 'UTC' or 'America/Los_Angeles', the physical values themselves are the same: changing the time zone only changes the metadata, not the values of the int64 timestamps

What is proposed here simplifies this to either isAdjustedToUTC=false (what we currently call "no time zone" or "time zone naive" in Arrow) or isAdjustedToUTC=true (which covers BOTH the case that the time zone is set as UTC or some other time zone)

The problem I see here is that if a data processing system runs the query:

select hour(timestamp_field), count(*) from my_parquet_data group by 1

For timestamps with time zone, if the time zone is known then the hour function can be implemented to compute the hour values based on the time zone (e.g. America/Los_Angeles). But what's proposed here precludes that, you would need to do something like

hour_localized(timestamp_field, 'America/Los_Angeles') ...

Or maybe the analytics system has some means to cast to a timestamp with the additional metadata. Either way there's some awkwardness.

This isn't done by other systems, and there is no guarantee that there is a single zone that the timestamps were converted from.

I see this concern, but if there is no consistency about the origin of the data, why not store "UTC" as the storage time zone? The fact that others may preserve a local time zone need not complicate this use case.

We can use the keyvalue metadata to preserve the time zone metadata in the Parquet file footer to at least maintain compatibility with Arrow, but it's not ideal.

We've resolved this discussion in the Parquet sync-ups. Parquet timestamps are always stored with respect to UTC and won't have a time zone.

julienledem · 2017-04-04T13:54:05Z

src/main/thrift/parquet.thrift

+
+/** Time logical type annotation */
+struct TimeType {
+  1: required bool isAdjustedToUTC


do this actually apply to Time?

Applies to at least PostgreSQL
https://www.postgresql.org/docs/9.1/static/datatype-datetime.html

Yes, this is required by the SQL spec

julienledem · 2017-04-04T13:55:02Z

src/main/thrift/parquet.thrift

+ * bitWidth must be 8, 16, 32, or 64.
+ */
+struct IntType {
+  1: required i32 bitWidth


byte instead of i32?

julienledem · 2017-04-04T13:55:20Z

src/main/thrift/parquet.thrift

+ */
+struct IntType {
+  1: required i32 bitWidth
+  2: required bool isSigned


just signed?

I'm fine either way. isSigned is the Java convention.

julienledem · 2017-04-04T13:58:26Z

src/main/thrift/parquet.thrift

+}
+
+/** Embedded logical type annotation */
+struct EmbeddedType {


I feel that we have too many layers here.
A Union is already replacing a notion of format field.

Sounds fine to me. I can add JsonType and BsonType instead.

majetideepak · 2017-04-14T03:35:19Z

+1 LGTM

majetideepak · 2017-04-14T03:37:06Z

I believe the physical type for Timestamp is only going to be INT64.

mkornacker · 2017-05-10T17:19:20Z

src/main/thrift/parquet.thrift

+  6:  DateType DATE           // use ConvertedType DATE
+  7:  TimeType TIME           // use ConvertedType TIME_MICROS or TIME_MILLIS
+  8:  TimestampType TIMESTAMP // use ConvertedType TIMESTAMP_MICROS or TIMESTAMP_MILLIS
+  // 9: reserved for INTERVAL


How about creating an empty struct IntervalType and leaving a todo with that struct instead?

but then we can not add required fields?

I agree with Julien. Then we can't add required fields to interval and there isn't much value to adding it now.

lekv · 2017-05-10T20:36:55Z

src/main/thrift/parquet.thrift

+/**
+ * Logical type to annotate a column that is always null.
+ *
+ * Sometimes when discovering the schema of existing data values are always


I found this sentence a bit tricky to parse. Can you add a comma to make it more clear?

I added some clarification here. Thanks for the suggestion.

lekv · 2017-05-10T20:39:42Z

src/main/thrift/parquet.thrift

+  2: MicroSeconds MICROS
+}
+
+/** Timestamp logical type annotation */


Could you add a sentence or two to the comments explaining the difference between TimestampType and TimeType? They're not clear to me.

These types are already documented in LogicalTypes.md. We can add some things here, like the allowed physical types, but I don't think we should maintain everything in two places.

lekv · 2017-05-10T20:40:44Z

src/main/thrift/parquet.thrift

+}
+
+/**
+ * Integer logical type annotation


Is this needed in addition to the physical types? Could it just be UnsignedIntType and leave out the bitWidth?

unless we want to support other widths in the future?

We should talk about this in the sync-up today.

I think I didn't make it to that sync. Was there some conclusion?

Bit width is needed to signal that all of the values will fit in a particular width. Since the spec currently has 8 and 16 bit widths, we have to capture those possible values.

This is consistent with the Arrow integer metadata https://github.com/apache/arrow/blob/master/format/Schema.fbs#L87

lekv · 2017-05-10T20:43:49Z

src/main/thrift/parquet.thrift

@@ -211,6 +204,93 @@ struct Statistics {
   4: optional i64 distinct_count;
 }

+/** Empty structs to use as logical type annotations */


I think think it would help the reader if the comments for each logical type mention the physical type(s) they can annotate. Currently one needs to look them up in the table below, and then look in LogicalTypes.md or ConvertedType to find what physical types can be annotated.

lekv · 2017-05-10T20:44:53Z

src/main/thrift/parquet.thrift

+  8:  TimestampType TIMESTAMP // use ConvertedType TIMESTAMP_MICROS or TIMESTAMP_MILLIS
+  // 9: reserved for INTERVAL
+  10: IntType INTEGER         // use ConvertedType INT_* or UINT_*
+  11: NullType NULL           // no compatible ConvertedType


Does the comment mean ", so leave it empty."?

Can we use NIL or NA here since NULL is a reserved keyword in C++?

I've updated this to NONE. Good suggestion.

I changed it to NONE, which is a little more common than NIL or NA. Python and Scala use it.

julienledem · 2017-05-12T01:42:51Z

src/main/thrift/parquet.thrift

@@ -211,6 +204,93 @@ struct Statistics {
   4: optional i64 distinct_count;
 }

+/** Empty structs to use as logical type annotations */


julienledem · 2017-05-12T01:45:32Z

src/main/thrift/parquet.thrift

@@ -211,6 +204,93 @@ struct Statistics {
   4: optional i64 distinct_count;
 }

+/** Empty structs to use as logical type annotations */
+struct StringType {}


add a comment that it is UTF8 by default ? (in case we want to add an encoding field at some point).

I can add a comment about UTF8, but I don't think we should allow other encodings, unless there is a new encoding that becomes the best standard, like UTF8 is today.

Added a comment about UTF-8

How about SQL engines that let the user put anything in a string without checking its encoding? Should they annotate strings with UTF8 even though they can not guarantee that users will actually store UTF8 in them, should they omit the annotation and thereby lose the information that those binary fields are actually strings, or should there be an encoding value for unknown string encoding? (See also: IMPALA-5982)

SQL engines that store arbitrary binary shouldn't mark it as UTF-8 because it isn't guaranteed to be. Non-UTF-8 characters would cause exceptions in Java. There are two follow-ups to this:

Start validating that data written by Impala is UTF-8 and add the UTF8 annotation to string columns. Scanning to verify an encoding shouldn't be too expensive.

Make sure that we can add annotations later in the expected schema. This is something we should add tests for in parquet-avro and other object models. It makes sense to me that converting one of these broken Parquet schemas to Avro shouldn't assume that binary columns can be read as Strings or Utf8 objects, but if I to read them that way by providing a schema with those fields as UTF8, I should be able to. Make sense?

julienledem · 2017-05-12T01:50:15Z

src/main/thrift/parquet.thrift

+}
+
+/**
+ * Integer logical type annotation


unless we want to support other widths in the future?

… Arrow Author: Julien Le Dem <[email protected]> Closes #45 from julienledem/types and squashes the following commits: 2956b63 [Julien Le Dem] review feedback 94236c4 [Julien Le Dem] PARQUET-757: Bring Parquet logical types to par with Arrow

rdblue · 2017-08-13T21:19:27Z

Updated and, I think, ready to commit? Any more comments?

lekv · 2017-09-13T16:10:36Z

+1

rdblue · 2017-09-26T15:46:57Z

@julienledem, any comments or do you think this is ready to merge?

rdblue · 2017-10-06T23:18:32Z

@julienledem, can you take one last look? I think this is ready to go in.

* Removed EmbeddedTypes, made JsonType and BsonType top-level * Changed IntType bitWidth to a byte

This hasn't been in a release, so there is no need to keep it, which is a forward-incompatbile change.

This fixes the Java test errors and uses UNKNOWN to avoid name conflicts.

julienledem

+1 LGTM

julienledem · 2017-10-10T18:11:04Z

maybe just review @lekv about the comment readability

UUIDs are commonly used as unique identifiers. A binary representation will reduce memory when writing or building bloom filters and will reduce cycles needed to compare values. This commit is based on PARQUET-906 / PR #51. Author: Ryan Blue <[email protected]> Closes #71 from rdblue/PARQUET-1125-add-uuid-logical-type and squashes the following commits: dc01707 [Ryan Blue] PARQUET-1125: Add UUID logical type.

zivanfi · 2017-11-10T10:54:31Z

src/main/thrift/parquet.thrift

+   * LogicalType replaces ConvertedType, but ConvertedType is still required
+   * for some logical types to ensure forward-compatibility in format v1.
+   */
+  10: optional LogicalType logicalType


I just noticed that this commit uses camelCase field names while the existing ones are underscore_separated. Was this an intentional naming convention change? In any case, it's too late to do anything about it as this has already been released as 2.4.0.

ConvertedType::NA corresponds to an invalid converted type that was once added to the Parquet spec: apache/parquet-format#45 but then quickly removed in favour of the Null logical type: apache/parquet-format#51 Unfortunately, Parquet C++ could still in some cases emit the unofficial converted type. Also remove the confusingly-named LogicalType::Unknown, while "UNKNOWN" in the Thrift specification points to LogicalType::Null.

ConvertedType::NA corresponds to an invalid converted type that was once added to the Parquet spec: apache/parquet-format#45 but then quickly removed in favour of the Null logical type: apache/parquet-format#51 Unfortunately, Parquet C++ could still in some cases emit the unofficial converted type. Also remove the confusingly-named LogicalType::Unknown, while "UNKNOWN" in the Thrift specification points to LogicalType::Null. Closes #9863 from pitrou/PARQUET-1990-invalid-converted-type Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>

ConvertedType::NA corresponds to an invalid converted type that was once added to the Parquet spec: apache/parquet-format#45 but then quickly removed in favour of the Null logical type: apache/parquet-format#51 Unfortunately, Parquet C++ could still in some cases emit the unofficial converted type. Also remove the confusingly-named LogicalType::Unknown, while "UNKNOWN" in the Thrift specification points to LogicalType::Null. Closes apache#9863 from pitrou/PARQUET-1990-invalid-converted-type Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>

rdblue mentioned this pull request Mar 31, 2017

PARQUET-686: Add Order to store the order used for min/max stats. #46

Closed

julienledem reviewed Apr 4, 2017

View reviewed changes

mkornacker reviewed May 10, 2017

View reviewed changes

lekv reviewed May 10, 2017

View reviewed changes

julienledem reviewed May 12, 2017

View reviewed changes

rdblue added 6 commits October 6, 2017 17:29

PARQUET-906: Add LogicalType annotation.

86a22b4

PARQUET-906: Remove the unreleased NULL ConvertedType.

993102e

PARQUET-906: Add copyright header to LogicalTypes.

8203b21

PARQUET-906: Update for review comments.

190bd8a

* Removed EmbeddedTypes, made JsonType and BsonType top-level * Changed IntType bitWidth to a byte

PARQUET-906: Remove NULL ConvertedType.

c0386e9

This hasn't been in a release, so there is no need to keep it, which is a forward-incompatbile change.

PARQUET-906: Update from comments on the PR.

02f3868

rdblue force-pushed the PARQUET-906-add-timestamp-adjustment-metadata branch from c997ac4 to 02f3868 Compare October 7, 2017 00:30

PARQUET-906: Rename NULL to UNKNOWN.

7cc29f7

This fixes the Java test errors and uses UNKNOWN to avoid name conflicts.

rdblue mentioned this pull request Oct 7, 2017

PARQUET-1125: Add UUID logical type. #71

Closed

julienledem approved these changes Oct 10, 2017

View reviewed changes

PARQUET-906: Clarify the use of NullType.

ad8e91d

asfgit closed this in 863875e Oct 10, 2017

zivanfi reviewed Nov 10, 2017

View reviewed changes

pitrou mentioned this pull request Mar 31, 2021

PARQUET-1990: [C++] Refuse to write ConvertedType::NA apache/arrow#9863

Closed

etseidl mentioned this pull request Jun 11, 2024

PARQUET-2310: implementation status apache/parquet-site#34

Merged

asfimport mentioned this pull request Jun 23, 2024

add logical type timestamp with timezone (per SQL) #322

Open

PARQUET-906: Add LogicalType annotation. #51

PARQUET-906: Add LogicalType annotation. #51

Conversation

rdblue commented Mar 24, 2017

rdblue commented Mar 24, 2017

julienledem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesm Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Apr 14, 2017

majetideepak commented Apr 14, 2017

Choose a reason for hiding this comment

julienledem May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Aug 13, 2017

lekv commented Sep 13, 2017

rdblue commented Sep 26, 2017

rdblue commented Oct 6, 2017

julienledem left a comment

Choose a reason for hiding this comment

julienledem commented Oct 10, 2017

Choose a reason for hiding this comment

wesm Jun 2, 2017 •

edited

Loading

julienledem May 12, 2017 •

edited

Loading