PARQUET-1253: Support for new logical type representation #463

nandorKollar · 2018-03-29T12:12:18Z

This PR implements the new logical type representation in parquet-mr which is already available in parquet-format. Reviews are welcome!

gszadovszky

Thanks for working on this.
I've had some comments in the related parts.
In addition, could you please take care of the code parts using the former OriginalType/ConvertedType classes by deprecating public API and using the new LogicalType classes instead of the former ones wherever it is possible? (If you prefer, create a separate JIRA for it.)

gszadovszky · 2018-04-03T11:59:52Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

+      this.precision = precision;
+    }
+
+    public void setPrecision(int precision) {


I don't really like this type being mutable. Is it possible to initialize the scale/precision values at construction only somehow?

gszadovszky · 2018-04-03T12:43:06Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

+    }
+  }
+
+  class NullLogicalTypeAnnotation implements LogicalTypeAnnotation {


I think, this implementation is not needed. All the cases where this instance is returned an exception should be thrown instead that the related case is unsupported.

gszadovszky · 2018-04-03T12:44:59Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

+    }
+  }
+
+  class IntervalLogicalTypeAnnotation implements LogicalTypeAnnotation {


This class is fake type as INTERVAL is not supported in LogicalTypes currently. I can see the purpose though. I would suggest adding some comments to make the purpose clear.

gszadovszky · 2018-04-03T13:16:40Z

parquet-column/src/main/java/org/apache/parquet/schema/Types.java

+      return self();
+    }
+
+    public THIS as(LogicalTypeAnnotation type) {


This method breaks the fluent API of the builder as you need to use an outside factory method of the final type to create a LogicalTypeAnnotation. If it is practically feasible, I would suggest to refactor the LogicalTypeAnnotation API to fit more in the fluent API of Types.

gszadovszky · 2018-04-03T13:22:07Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

+    }
+  }
+
+  LogicalTypeAnnotation.TimeUnit convertTimeUnit(TimeUnit unit) {


As far as I can see this method can be private. Please, use visibility as tight as possible and comment if something is visible for testing purposes only.

gszadovszky · 2018-04-03T13:24:52Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/metadata/ParquetMetadata.java

@@ -41,6 +40,10 @@

  private static final ObjectMapper objectMapper = new ObjectMapper();

+  static {
+    objectMapper.configure(SerializationConfig.Feature.FAIL_ON_EMPTY_BEANS, false);


Could you please explain why it is necessary?

Without this configuration change on the objectMapper three tests in parquet-cascading3 fail with the following error:

testReadPattern(org.apache.parquet.cascading.TestParquetTupleScheme): [namecp] could not build flow from assembly: [java.io.IOException: Could not read footer: java.lang.RuntimeException: org.codehaus.jackson.map.JsonMappingException: No serializer found for class org.apache.parquet.schema.LogicalTypeAnnotation$StringLogicalTypeAnnotation and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationConfig.Feature.FAIL_ON_EMPTY_BEANS) ) (through reference chain: org.apache.parquet.hadoop.metadata.ParquetMetadata["fileMetaData"]->org.apache.parquet.hadoop.metadata.FileMetaData["schema"]->org.apache.parquet.schema.MessageType["fields"]->java.util.ArrayList[0]->org.apache.parquet.schema.PrimitiveType["logicalTypeAnnotation"])]

The reason is: the logical types are no longer represented as enum, but a classed, and Jackson can't serialize empty classes because FAIL_ON_EMPTY_BEANS feature is enabled by default.

I see. Thanks.
Could you please add some comments about that so others will also understand?

Sure, no problem, I will.

gszadovszky · 2018-04-18T14:38:25Z

parquet-column/src/main/java/org/apache/parquet/schema/GroupType.java

@@ -88,6 +88,7 @@ public GroupType(Repetition repetition, String name, OriginalType originalType,
   * @param fields the contained fields
   * @param id the id of the field
   */
+  @Deprecated


I think, it is enough to deprecate public API. If a method is not public we can freely modify/remove it.

gszadovszky · 2018-04-18T14:41:25Z

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java

  }

  /**
   * @param repetition OPTIONAL, REPEATED, REQUIRED
   * @param primitive STRING, INT64, ...
   * @param name the name of the type
   * @param originalType (optional) the original type to help with cross schema convertion (LIST, MAP, ...)
+   *
+   * @deprecated use {@link #PrimitiveType(Repetition, PrimitiveTypeName, String, LogicalTypeAnnotation)} instead


The usual pattern in parquet for deprecating is to mention that the related method will be removed in 2.0.0. In case of the suggestion would be to use one of the overloaded methods then it is fine to not mention.

gszadovszky · 2018-04-18T14:42:28Z

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java

@@ -436,13 +438,20 @@ public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
   * @param originalType (optional) the original type (MAP, DECIMAL, UTF8, ...)
   * @param decimalMeta (optional) metadata about the decimal type
   * @param id the id of the field
+   *
+   * @deprecated use {@link #PrimitiveType(Repetition, PrimitiveTypeName, int, String, LogicalTypeAnnotation, ID)} instead


gszadovszky · 2018-04-18T14:42:44Z

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java

  public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
                       int length, String name, OriginalType originalType,
                       DecimalMetadata decimalMeta, ID id) {
    this(repetition, primitive, length, name, originalType, decimalMeta, id, null);
  }

+  /**
+   * @deprecated use {@link #PrimitiveType(Repetition, PrimitiveTypeName, int, String, LogicalTypeAnnotation, ID, ColumnOrder)} instead


Not public, no need for deprecation.

gszadovszky · 2018-04-18T14:42:55Z

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java

+  /**
+   * @deprecated use {@link #PrimitiveType(Repetition, PrimitiveTypeName, int, String, LogicalTypeAnnotation, ID, ColumnOrder)} instead
+   */
+  @Deprecated


gszadovszky · 2018-04-18T14:52:27Z

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java

@@ -459,6 +468,37 @@ public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
    this.columnOrder = requireValidColumnOrder(columnOrder);
  }

+  public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,


I think, in long term we do not want to expose the construction of types from the API. We would like the clients to use the builder instead. Therefore, I would suggest not adding public constructors if your code does not need it.

What do you think about my previous comment? If you agree, you should say at the deprecations to use the Types factory instead of directly creating PrimitiveType objects.

Ah sorry, missed this one. Sure, I can make it package private

gszadovszky · 2018-04-18T14:55:57Z

parquet-column/src/main/java/org/apache/parquet/schema/Type.java

   */
+  @Deprecated


Not public, no deprecating is required.

gszadovszky · 2018-04-18T14:56:07Z

parquet-column/src/main/java/org/apache/parquet/schema/Type.java

+  /**
+   * @deprecated use {@link #Type(String, Repetition, LogicalTypeAnnotation, ID)} instead
+   */
+  @Deprecated


gszadovszky · 2018-04-20T10:02:31Z

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java

@@ -459,6 +468,37 @@ public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
    this.columnOrder = requireValidColumnOrder(columnOrder);
  }

+  public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,


What do you think about my previous comment? If you agree, you should say at the deprecations to use the Types factory instead of directly creating PrimitiveType objects.

gszadovszky · 2018-04-20T10:17:52Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

+    private final byte bitWidth;
+    private final boolean isSigned;
+
+    public static LogicalTypeAnnotation create(byte bitWidth, boolean isSigned) {


Because you expect a byte for bitWidth the caller has to cast the value all the time. I think, there is no reason expecting a byte here so, I would suggest using an int instead.

IntType in parquet-format expects byte as bitWidth, thus if this method would accept int instead of byte, then in line 503 (LogicalType.INTEGER(new IntType(bitWidth, isSigned))) we should downcast bitWidth to byte. Is this fine, should I downcast there?

I think it is better to do the cast in one place so the user don't have to do it every time when using this API. The cast would be safe as only the values 8, 16, 32 and 64 are allowed. By the way, I would be good to check if really one of these values are passed.

gszadovszky · 2018-04-20T10:23:37Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

+  class StringLogicalTypeAnnotation implements LogicalTypeAnnotation {
+    private static final StringLogicalTypeAnnotation INSTANCE = new StringLogicalTypeAnnotation();
+
+    public static LogicalTypeAnnotation create() {


I don't really like the pattern how the client has to write the whole classnames of the specialized LogicalTypeAnnotataion implementations to create one. What do you think about having named static factory methods one level up instead?
For example instead of calling LogicalTypeAnnotation.StringLogicalTypeAnnotation.create() you might call LogicalTypeAnnotation.string() or more simply string() if static import is used.

…mment related to byte-int casting

gszadovszky

Added one comment. Otherwise, seems good to me.

gszadovszky · 2018-04-24T16:33:59Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

+    return BsonLogicalTypeAnnotation.INSTANCE;
+  }
+
+  static IntervalLogicalTypeAnnotation intervalType() {


Do we want the user to be able to create an Interval or a MapKeyValue type? If they are only exist for backward compatibility reasons the factory method should not be public so users won't be able to create them.

I think MapKeyValue is here only for backward compatibility, but Interval should be supported in the future. Since these factory methods are defined on an interface, I can't easily change the visibility to private/package protected. Should I change it to an abstract class?

Interval is not supported in LogicalType currently, so I would not expose it yet. It is always possible to make a restricted access public but in the other way it would break backward compatibility.
Abstract class sounds reasonable to me.

This commit * Address code review changes: replace Interval and KeyValue logical type factory methods with methods on logical type to indicate that they're not intended to be used publicly * Incorporate new parameters for logical types for timestamp, time and integer

gszadovszky

Thanks a lot for the hard work you already invested in this change!
I would like you to keep up the good work so I added some additional comments ;) sorry...
Could you please also investigate the Travis failures and make it pass?

gszadovszky · 2018-05-08T15:52:12Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

 import java.util.Objects;

-public interface LogicalTypeAnnotation {
+public abstract class LogicalTypeAnnotation {
+  public enum LogicalTypes {


This enum is used only for parsing/printing and we don't want the users to really use them. So, I would suggest using a name that suggests its use e.g. LogicalTypeParseHelper?
Also, it would be nice if we could annotate/comment that this one is not part of the public API.

gszadovszky · 2018-05-08T15:55:21Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

  /**
   * Convert this parquet-mr logical type to parquet-format LogicalType.
   *
   * @return the parquet-format LogicalType representation of this logical type implementation
   */
-  LogicalType toLogicalType();
+  public abstract LogicalType toLogicalType();


We usually try to keep the usage of the thrift generated objects (from parquet-format) in ParquetMetadataConverter. Do you think it is possible to refactor the related code parts to there (not necessarily to the same class but to the same module)?

gszadovszky · 2018-05-08T15:57:59Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

-  void accept(LogicalTypeAnnotationVisitor logicalTypeAnnotationVisitor);
+  public abstract void accept(LogicalTypeAnnotationVisitor logicalTypeAnnotationVisitor);
+
+  public abstract LogicalTypes getType();


It seems this one is not used outside of this class/package. I would suggest not being public.

accept is intended for public use, for the visitors

Sorry, I meant getType().

gszadovszky · 2018-05-08T15:58:50Z

parquet-column/src/test/java/org/apache/parquet/parser/TestParquetParser.java

+
+    MessageType parsed = MessageTypeParser.parseMessageType(message);
+    MessageType expected = Types.buildMessage()
+      .required(INT32).as(LogicalTypeAnnotation.intType(8, true)).named("i8")


nit: I think, the static import of the method intType would help reading the code.

nandorKollar · 2018-05-09T09:27:51Z

@gszadovszky it looks like the version of maven-shade-plugin and enforcer-rule dependency in maven-enforcer-plugin Maven plugins used in Parquet don't like Java 8 lambda expressions, that's why Travis build failed. Since Parquet is already upgraded to Java 8, I think we should upgrade these plugins. I'll create a separate Jira for this issue.

gszadovszky

Thanks a lot for the changes. 2 more to go ;)
After fixing these, I'll be fine with this.
Thanks again for the efforts

gszadovszky · 2018-05-14T07:30:17Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java


 import java.util.List;
 import java.util.Objects;

 public abstract class LogicalTypeAnnotation {
-  public enum LogicalTypes {
+  // This is a private enum intended only for internal use for parsing the schema
+  public enum LogicalTypeToken {


As far as I can see this enum is used only from the schema package. I would suggest using package private access so the comment is not needed and the clients cannot misuse it.

Good idea, thanks Gabor!

gszadovszky · 2018-05-14T07:37:03Z

parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java

@@ -164,7 +135,7 @@ protected LogicalTypeAnnotation fromString(List<String> params) {
   */
  public abstract void accept(LogicalTypeAnnotationVisitor logicalTypeAnnotationVisitor);

-  public abstract LogicalTypes getType();
+  protected abstract LogicalTypeToken getType();


nit: package private would be fine here.

Indeed, thanks! I'm wondering, if we should narrow down the scope of a bunch of other methods to package private in LogicalTypeAnnotation too? For example, toOriginalType is only used within the same package, apart from one test case, not sure that it should be part of public API.

Good point. I would say if something is not necessary to be public then restrict its access before committing. The later the harder to remove from the public API.

wesm · 2018-11-21T15:21:38Z

What will happen if you try reading a Parquet file with the new logical types with an old reader (or another implementation, like Apache Impala)?

nandorKollar · 2018-11-21T17:27:06Z

@wesm old logical types (ConvertedTypes in Thrift schema) are still written as long as the LogicalType can be paired with a ConvertedType. For those logical type, which already exist, this mapping is documented here: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md, old readers should be able to read these Thrift schemas as they did before, because the expected ConvertedType field is populated.

For those types, which are introduced later, for example nanosecond precision timestamp, we decided not to introduce a ConvertedType, but just add a new TimeUnit to Timestamp and Time logical types. In this case, the ConvertedType field is not populated at all, therefore old readers will just see the physical type. Does this answer your question?

nkollar added 3 commits March 29, 2018 00:51

PARQUET-1253: Support for new logical type representation

244bb6a

refactor: move conversion logic to logical type classes

0a66346

better (hopefully) name for new type

f8e2236

gszadovszky requested changes Apr 3, 2018

View reviewed changes

nkollar added 2 commits April 5, 2018 11:53

Address code review comments

6b4ff74

Deprecate old constructors, address comment related to builder

f801605

gszadovszky requested changes Apr 18, 2018

View reviewed changes

Address latest comments

db30adb

gszadovszky requested changes Apr 20, 2018

View reviewed changes

nkollar added 2 commits April 24, 2018 17:37

Introduce type visitors for logical types, and address code review co…

047feb9

…mment related to byte-int casting

Move factory methods to the interface for each logical type subclass

d11a09c

gszadovszky requested changes Apr 24, 2018

View reviewed changes

gszadovszky requested changes May 8, 2018

View reviewed changes

Address latest code review comments

3c426d9

gszadovszky requested changes May 14, 2018

View reviewed changes

nkollar added 2 commits May 14, 2018 16:34

Make LogicalTypeToken enum and getType method package private

77f1d52

Make typeParametersAsString package private

6e1ea5d

gszadovszky approved these changes May 16, 2018

View reviewed changes

gszadovszky merged commit 94a8bf6 into apache:master May 24, 2018

gszadovszky mentioned this pull request Jul 4, 2018

PARQUET-1335: Logical type names in parquet-mr are not consistent with parquet-format #496

Merged

ghost pushed a commit to RMS/parquet-mr that referenced this pull request Aug 18, 2018

PARQUET-1253: Support for new logical type representation (apache#463)

89861a3

ryanrupp mentioned this pull request Dec 2, 2019

Use timestamp statistics trinodb/trino#1999

Closed

zhangjiashen mentioned this pull request Sep 17, 2023

PARQUET-1647: [Java][Parquet] Implement FLOAT16 logical type #1142

Merged

4 tasks

asfimport mentioned this pull request Jun 23, 2024

Support for new logical type representation #1512

Closed

PARQUET-1253: Support for new logical type representation #463

PARQUET-1253: Support for new logical type representation #463

Conversation

nandorKollar commented Mar 29, 2018

gszadovszky 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

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

gszadovszky Apr 20, 2018 • 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

gszadovszky Apr 20, 2018 • 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

gszadovszky 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

gszadovszky 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

nandorKollar commented May 9, 2018

gszadovszky 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

wesm commented Nov 21, 2018

nandorKollar commented Nov 21, 2018

gszadovszky Apr 20, 2018 •

edited

Loading

gszadovszky Apr 20, 2018 •

edited

Loading