Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
/*
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
*
* http://www.apache.org/licenses/LICENSE-2.0
*
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
Expand Down Expand Up @@ -47,7 +47,7 @@ public class GroupType extends Type {
* @param fields the contained fields
*/
public GroupType(Repetition repetition, String name, List<Type> fields) {
this(repetition, name, null, fields, null);
this(repetition, name, (LogicalTypeAnnotation) null, fields, null);
}

/**
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GroupType(Repetition repetition, String name, OriginalType originalType, List<Type> fields, ID id) {
super(name, repetition, originalType, id);
this.fields = fields;
Expand All @@ -97,6 +98,15 @@ public GroupType(Repetition repetition, String name, OriginalType originalType,
}
}

GroupType(Repetition repetition, String name, LogicalTypeAnnotation logicalTypeAnnotation, List<Type> fields, ID id) {
super(name, repetition, logicalTypeAnnotation, id);
this.fields = fields;
this.indexByName = new HashMap<String, Integer>();
for (int i = 0; i < fields.size(); i++) {
indexByName.put(fields.get(i).getName(), i);
}
}

/**
* @param id the field id
* @return a new GroupType with the same fields and a new id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,14 @@ private DecimalLogicalTypeAnnotation(int scale, int precision) {
this.precision = precision;
}

public int getPrecision() {
return precision;
}

public int getScale() {
return scale;
}

@Override
public LogicalType toLogicalType() {
return LogicalType.DECIMAL(new DecimalType(scale, precision));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,9 +389,8 @@ abstract public void addValueToPrimitiveConverter(
* @param primitive STRING, INT64, ...
* @param name the name of the type
*/
public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
String name) {
this(repetition, primitive, 0, name, null, null, null);
public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive, String name) {
this(repetition, primitive, 0, name, (LogicalTypeAnnotation) null, null, null);
}

/**
Expand All @@ -401,15 +400,18 @@ public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
* @param name the name of the type
*/
public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive, int length, String name) {
this(repetition, primitive, length, name, null, null, null);
this(repetition, primitive, length, name, (LogicalTypeAnnotation) null, null, null);
}

/**
* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
@Deprecated
public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
String name, OriginalType originalType) {
this(repetition, primitive, 0, name, originalType, null, null);
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

*/
@Deprecated
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not public, no need for deprecation.

*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
int length, String name, OriginalType originalType,
DecimalMetadata decimalMeta, ID id, ColumnOrder columnOrder) {
Expand All @@ -459,6 +468,37 @@ public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
this.columnOrder = requireValidColumnOrder(columnOrder);
}

public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@gszadovszky gszadovszky Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

String name, LogicalTypeAnnotation logicalTypeAnnotation) {
this(repetition, primitive, 0, name, logicalTypeAnnotation, null, null);
}

public PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
int length, String name, LogicalTypeAnnotation logicalTypeAnnotation, ID id) {
this(repetition, primitive, length, name, logicalTypeAnnotation, id, null);
}

PrimitiveType(Repetition repetition, PrimitiveTypeName primitive,
int length, String name, LogicalTypeAnnotation logicalTypeAnnotation,
ID id, ColumnOrder columnOrder) {
super(name, repetition, logicalTypeAnnotation, id);
this.primitive = primitive;
this.length = length;
if (getOriginalType() == OriginalType.DECIMAL) {
LogicalTypeAnnotation.DecimalLogicalTypeAnnotation decimal = (LogicalTypeAnnotation.DecimalLogicalTypeAnnotation) logicalTypeAnnotation;
this.decimalMeta = new DecimalMetadata(decimal.getPrecision(), decimal.getScale());
} else {
this.decimalMeta = null;
}

if (columnOrder == null) {
columnOrder = primitive == PrimitiveTypeName.INT96 || getOriginalType() == OriginalType.INTERVAL
? ColumnOrder.undefined()
: ColumnOrder.typeDefined();
}
this.columnOrder = requireValidColumnOrder(columnOrder);
}

private ColumnOrder requireValidColumnOrder(ColumnOrder columnOrder) {
if (primitive == PrimitiveTypeName.INT96) {
Preconditions.checkArgument(columnOrder.getColumnOrderName() == ColumnOrderName.UNDEFINED,
Expand Down
21 changes: 20 additions & 1 deletion parquet-column/src/main/java/org/apache/parquet/schema/Type.java
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public boolean isMoreRestrictiveThan(Repetition other) {
*/
@Deprecated
public Type(String name, Repetition repetition) {
this(name, repetition, null, null);
this(name, repetition, (LogicalTypeAnnotation) null, null);
}

/**
Expand All @@ -146,11 +146,18 @@ public Type(String name, Repetition repetition, OriginalType originalType) {
* @param repetition OPTIONAL, REPEATED, REQUIRED
* @param originalType (optional) the original type to help with cross schema conversion (LIST, MAP, ...)
* @param id (optional) the id of the fields.
*
* @deprecated use {@link #Type(String, Repetition, LogicalTypeAnnotation, ID)} instead
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not public, no deprecating is required.

Type(String name, Repetition repetition, OriginalType originalType, ID id) {
this(name, repetition, originalType, null, id);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Type(String name, Repetition repetition, OriginalType originalType, DecimalMetadata decimalMetadata, ID id) {
super();
this.name = checkNotNull(name, "name");
Expand All @@ -159,6 +166,18 @@ public Type(String name, Repetition repetition, OriginalType originalType) {
this.id = id;
}

public Type(String name, Repetition repetition, LogicalTypeAnnotation logicalTypeAnnotation) {
this(name, repetition, logicalTypeAnnotation, null);
}

Type(String name, Repetition repetition, LogicalTypeAnnotation logicalTypeAnnotation, ID id) {
super();
this.name = checkNotNull(name, "name");
this.repetition = checkNotNull(repetition, "repetition");
this.logicalTypeAnnotation = logicalTypeAnnotation;
this.id = id;
}

/**
* @param id
* @return the same type with the id field set
Expand Down
49 changes: 46 additions & 3 deletions parquet-column/src/main/java/org/apache/parquet/schema/Types.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.List;

import org.apache.parquet.Preconditions;
import org.apache.parquet.format.DecimalType;
import org.apache.parquet.schema.ColumnOrder.ColumnOrderName;
import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName;
import org.apache.parquet.schema.Type.ID;
Expand Down Expand Up @@ -250,14 +251,32 @@ protected final THIS repetition(Type.Repetition repetition) {
*
* @param type an {@code OriginalType}
* @return this builder for method chaining
*
* @deprecated use {@link #as(LogicalTypeAnnotation)} with the corresponding logical type instead
*/
@Deprecated
public THIS as(OriginalType type) {
this.logicalTypeAnnotation = LogicalTypeAnnotation.fromOriginalType(type, null);
return self();
}

protected boolean newLogicalTypeSet;

/**
* Adds a type annotation ({@link LogicalTypeAnnotation}) to the type being built.
* <p>
* Type annotations are used to extend the types that parquet can store, by
* specifying how the primitive types should be interpreted. This keeps the
* set of primitive types to a minimum and reuses parquet's efficient
* encodings. For example, strings are stored as byte arrays (binary) with
* a UTF8 annotation.
*
* @param type an {@code {@link LogicalTypeAnnotation}}
* @return this builder for method chaining
*/
public THIS as(LogicalTypeAnnotation type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.logicalTypeAnnotation = type;
this.newLogicalTypeSet = true;
return self();
}

Expand Down Expand Up @@ -351,6 +370,9 @@ public THIS length(int length) {
return self();
}

private boolean precisionAlreadySet;
private boolean scaleAlreadySet;

/**
* Adds the precision for a DECIMAL.
* <p>
Expand All @@ -360,9 +382,13 @@ public THIS length(int length) {
*
* @param precision an int precision value for the DECIMAL
* @return this builder for method chaining
*
* @deprecated use {@link #as(LogicalTypeAnnotation)} with the corresponding decimal type instead
*/
@Deprecated
public THIS precision(int precision) {
this.precision = precision;
precisionAlreadySet = true;
return self();
}

Expand All @@ -378,9 +404,13 @@ public THIS precision(int precision) {
*
* @param scale an int scale value for the DECIMAL
* @return this builder for method chaining
*
* @deprecated use {@link #as(LogicalTypeAnnotation)} with the corresponding decimal type instead
*/
@Deprecated
public THIS scale(int scale) {
this.scale = scale;
scaleAlreadySet = true;
return self();
}

Expand Down Expand Up @@ -498,11 +528,24 @@ private static long maxPrecision(int numBytes) {
protected DecimalMetadata decimalMetadata() {
DecimalMetadata meta = null;
if (OriginalType.DECIMAL == getOriginalType()) {
LogicalTypeAnnotation.DecimalLogicalTypeAnnotation decimalType = (LogicalTypeAnnotation.DecimalLogicalTypeAnnotation) logicalTypeAnnotation;
if (newLogicalTypeSet) {
if (scaleAlreadySet) {
Preconditions.checkArgument(this.scale == decimalType.getScale(),
"Decimal scale should match with the scale of the logical type");
}
if (precisionAlreadySet) {
Preconditions.checkArgument(this.precision == decimalType.getPrecision(),
"Decimal precision should match with the precision of the logical type");
}
scale = decimalType.getScale();
precision = decimalType.getPrecision();
}
Preconditions.checkArgument(precision > 0,
"Invalid DECIMAL precision: " + precision);
Preconditions.checkArgument(scale >= 0,
"Invalid DECIMAL scale: " + scale);
Preconditions.checkArgument(scale <= precision,
Preconditions.checkArgument(this.scale >= 0,
"Invalid DECIMAL scale: " + this.scale);
Preconditions.checkArgument(this.scale <= precision,
"Invalid DECIMAL scale: cannot be greater than precision");
meta = new DecimalMetadata(precision, scale);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
/*
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
*
* http://www.apache.org/licenses/LICENSE-2.0
*
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
Expand Down Expand Up @@ -1396,6 +1396,47 @@ public PrimitiveType call() {
});
}

@Test
public void testDecimalLogicalType() {
PrimitiveType expected = new PrimitiveType(REQUIRED, BINARY, "aDecimal",
LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4));
PrimitiveType actual = Types.required(BINARY)
.as(LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4)).named("aDecimal");
Assert.assertEquals(expected, actual);
}

@Test
public void testDecimalLogicalTypeWithDeprecatedScale() {
PrimitiveType expected = new PrimitiveType(REQUIRED, BINARY, "aDecimal",
LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4));
PrimitiveType actual = Types.required(BINARY)
.as(LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4)).scale(3).named("aDecimal");
Assert.assertEquals(expected, actual);
}

@Test
public void testDecimalLogicalTypeWithDeprecatedPrecision() {
PrimitiveType expected = new PrimitiveType(REQUIRED, BINARY, "aDecimal",
LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4));
PrimitiveType actual = Types.required(BINARY)
.as(LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4)).precision(4).named("aDecimal");
Assert.assertEquals(expected, actual);
}

@Test(expected = IllegalArgumentException.class)
public void testDecimalLogicalTypeWithDeprecatedScaleMismatch() {
Types.required(BINARY)
.as(LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4))
.scale(4).named("aDecimal");
}

@Test(expected = IllegalArgumentException.class)
public void testDecimalLogicalTypeWithDeprecatedPrecisionMismatch() {
Types.required(BINARY)
.as(LogicalTypeAnnotation.DecimalLogicalTypeAnnotation.create(3, 4))
.precision(5).named("aDecimal");
}

/**
* A convenience method to avoid a large number of @Test(expected=...) tests
* @param message A String message to describe this assertion
Expand Down
Loading