wgtmac commented on code in PR #1142:
URL: https://github.com/apache/parquet-mr/pull/1142#discussion_r1337924016
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Statistics.java:
##########
@@ -139,6 +140,43 @@ public Statistics<?> build() {
}
}
+ // Builder for FLOAT16 type to handle special cases of min/max values like
NaN, -0.0, and 0.0
+ private static class Float16Builder extends Builder {
+ public Float16Builder(PrimitiveType type) {
+ super(type);
+ assert type.getPrimitiveTypeName() == PrimitiveTypeName.BINARY;
Review Comment:
```suggestion
assert type.getPrimitiveTypeName() ==
PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY;
```
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Statistics.java:
##########
@@ -139,6 +140,43 @@ public Statistics<?> build() {
}
}
+ // Builder for FLOAT16 type to handle special cases of min/max values like
NaN, -0.0, and 0.0
+ private static class Float16Builder extends Builder {
+ public Float16Builder(PrimitiveType type) {
+ super(type);
+ assert type.getPrimitiveTypeName() == PrimitiveTypeName.BINARY;
+ }
+
+ @Override
+ public Statistics<?> build() {
+ Float16Statistics stats = (Float16Statistics) super.build();
+ if (stats.hasNonNullValue()) {
+ Binary bMin = stats.genericGetMin();
+ Binary bMax = stats.genericGetMax();
+ short min = Float16.fromBytesLittleEndian(bMin.getBytes());
+ short max = Float16.fromBytesLittleEndian(bMax.getBytes());
+ // Drop min/max values in case of NaN as the sorting order of values
is undefined for this case
+ if (Float16.isNaN(min) || Float16.isNaN(max)) {
+ bMin =
Binary.fromConstantByteArray(Float16.toBytesLittleEndian(Float16.POSITIVE_ZERO));
Review Comment:
It seems worth adding two static constants (of Binary type) to Float16 for
POSITIVE_ZERO and NEGATIVE_ZERO as they are repeatedly constructed.
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Statistics.java:
##########
@@ -226,6 +268,11 @@ public static Builder getBuilderForReading(PrimitiveType
type) {
return new FloatBuilder(type);
case DOUBLE:
return new DoubleBuilder(type);
+ case BINARY:
Review Comment:
```suggestion
case FIXED_LEN_BYTE_ARRAY:
```
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Float16Statistics.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.parquet.column.statistics;
+
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.apache.parquet.schema.PrimitiveType;
+import org.apache.parquet.schema.Types;
+
+public class Float16Statistics extends BinaryStatistics
+{
+ // A fake type object to be used to generate the proper comparator
+ private static final PrimitiveType DEFAULT_FAKE_TYPE =
Types.optional(PrimitiveType.PrimitiveTypeName.BINARY)
+
.named("fake_binary_float16_type").withLogicalTypeAnnotation(LogicalTypeAnnotation.float16Type());
+
+ /**
+ * @deprecated will be removed in 2.0.0. Use {@link
Statistics#createStats(org.apache.parquet.schema.Type)} instead
+ */
+ @Deprecated
Review Comment:
We shouldn't even add this if it is a deprecated one.
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Statistics.java:
##########
@@ -139,6 +140,43 @@ public Statistics<?> build() {
}
}
+ // Builder for FLOAT16 type to handle special cases of min/max values like
NaN, -0.0, and 0.0
+ private static class Float16Builder extends Builder {
+ public Float16Builder(PrimitiveType type) {
+ super(type);
+ assert type.getPrimitiveTypeName() == PrimitiveTypeName.BINARY;
Review Comment:
Please check the fixed length (2) as well.
##########
parquet-common/src/main/java/org/apache/parquet/type/Float16.java:
##########
@@ -0,0 +1,339 @@
+/**
+ * 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
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.type;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+
+/**
+ * The class is a utility class to manipulate half-precision 16-bit
+ * <a
href="https://en.wikipedia.org/wiki/Half-precision_floating-point_format";>IEEE
754</a>
+ * floating point data types (also called fp16 or binary16). A half-precision
float can be
+ * created from or converted to single-precision floats, and is stored in a
short data type.
+ * The IEEE 754 standard specifies an float16 as having the following format:
+ * <ul>
+ * <li>Sign bit: 1 bit</li>
+ * <li>Exponent width: 5 bits</li>
+ * <li>Significand: 10 bits</li>
+ * </ul>
+ *
+ * <p>The format is laid out as follows:</p>
+ * <pre>
+ * 1 11111 1111111111
+ * ^ --^-- -----^----
+ * sign | |_______ significand
+ * |
+ * -- exponent
+ * </pre>
+ * Half-precision floating points can be useful to save memory and/or
+ * bandwidth at the expense of range and precision when compared to
single-precision
+ * floating points (float32).
+ * Ref:
https://android.googlesource.com/platform/libcore/+/master/luni/src/main/java/libcore/util/FP16.java
+ */
+public class Float16
+{
+ // Smallest negative value a half-precision float may have.
+ public static final short LOWEST_VALUE = (short) 0xfbff;
+ // Maximum positive finite value a half-precision float may have.
+ public static final short MAX_VALUE = (short) 0x7bff;
+ // Smallest positive normal value a half-precision float may have.
+ public static final short MIN_NORMAL = (short) 0x0400;
+ // Smallest positive non-zero value a half-precision float may have.
+ public static final short MIN_VALUE = (short) 0x0001;
+ // Positive 0 of type half-precision float.
+ public static final short POSITIVE_ZERO = (short) 0x0000;
+ // Negative 0 of type half-precision float.
+ public static final short NEGATIVE_ZERO = (short) 0x8000;
+ // A Not-a-Number representation of a half-precision float.
+ static final short NaN = (short) 0x7e00;
+ // Positive infinity of type half-precision float.
+ static final short POSITIVE_INFINITY = (short) 0x7c00;
+ // Negative infinity of type half-precision float.
+ static final short NEGATIVE_INFINITY = (short) 0xfc00;
+
+ // The bitmask to and a number with to obtain the sign bit.
+ private static final int SIGN_MASK = 0x8000;
+ // The offset to shift by to obtain the exponent bits.
+ private static final int EXPONENT_SHIFT = 10;
+ // The bitmask to and a number shifted by EXPONENT_SHIFT right, to obtain
exponent bits.
+ private static final int SHIFTED_EXPONENT_MASK = 0x1f;
+ // The bitmask to and a number with to obtain significand bits.
+ private static final int SIGNIFICAND_MASK = 0x3ff;
+ // The offset of the exponent from the actual value.
+ private static final int EXPONENT_BIAS = 15;
+ // The offset to shift by to obtain the sign bit.
+ private static final int SIGN_SHIFT = 15;
+ // The bitmask to AND with to obtain exponent and significand bits.
+ private static final int EXPONENT_SIGNIFICAND_MASK = 0x7fff;
+
+ private static final int FP32_SIGN_SHIFT = 31;
+ private static final int FP32_EXPONENT_SHIFT = 23;
+ private static final int FP32_SHIFTED_EXPONENT_MASK = 0xff;
+ private static final int FP32_SIGNIFICAND_MASK = 0x7fffff;
+ private static final int FP32_EXPONENT_BIAS = 127;
+ private static final int FP32_QNAN_MASK = 0x400000;
+ private static final int FP32_DENORMAL_MAGIC = 126 << 23;
+ private static final float FP32_DENORMAL_FLOAT =
Float.intBitsToFloat(FP32_DENORMAL_MAGIC);
+
+ /**
+ * Converts the specified half-precision float value into a
+ * single-precision float value. The following special cases are handled:
+ * If the input is NaN, the returned value is Float NaN.
+ * If the input is POSITIVE_INFINITY or NEGATIVE_INFINITY, the returned
value is respectively
+ * Float POSITIVE_INFINITY or Float NEGATIVE_INFINITY.
+ * If the input is 0 (positive or negative), the returned value is +/-0.0f.
+ * Otherwise, the returned value is a normalized single-precision float
value.
+ *
+ * @param h The half-precision float value to convert to single-precision
+ * @return A normalized single-precision float value
+ */
+ public static float toFloat(short h) {
+ int bits = h & 0xffff;
+ int s = bits & SIGN_MASK;
+ int e = (bits >>> EXPONENT_SHIFT) & SHIFTED_EXPONENT_MASK;
+ int m = (bits ) & SIGNIFICAND_MASK;
+ int outE = 0;
+ int outM = 0;
+ if (e == 0) { // Denormal or 0
+ if (m != 0) {
+ // Convert denorm fp16 into normalized fp32
+ float o = Float.intBitsToFloat(FP32_DENORMAL_MAGIC + m);
+ o -= FP32_DENORMAL_FLOAT;
+ return s == 0 ? o : -o;
+ }
+ } else {
+ outM = m << 13;
+ if (e == 0x1f) { // Infinite or NaN
+ outE = 0xff;
+ if (outM != 0) { // SNaNs are quieted
+ outM |= FP32_QNAN_MASK;
+ }
+ } else {
+ outE = e - EXPONENT_BIAS + FP32_EXPONENT_BIAS;
+ }
+ }
+ int out = (s << 16) | (outE << FP32_EXPONENT_SHIFT) | outM;
+ return Float.intBitsToFloat(out);
+ }
+
+ /**
+ * Converts the specified single-precision float value into a
+ * half-precision float value. The following special cases are handled:
+ *
+ * If the input is NaN, the returned value is NaN.
+ * If the input is Float POSITIVE_INFINITY or Float NEGATIVE_INFINITY,
+ * the returned value is respectively POSITIVE_INFINITY or
NEGATIVE_INFINITY.
+ * If the input is 0 (positive or negative), the returned value is
+ * POSITIVE_ZERO or NEGATIVE_ZERO.
+ * If the input is a less than MIN_VALUE, the returned value
+ * is flushed to POSITIVE_ZERO or NEGATIVE_ZERO.
+ * If the input is a less than MIN_NORMAL, the returned value
+ * is a denorm half-precision float.
+ * Otherwise, the returned value is rounded to the nearest
+ * representable half-precision float value.
+ *
+ * @param f The single-precision float value to convert to half-precision
+ * @return A half-precision float value
+ */
+ public static short toFloat16(float f) {
+ int bits = Float.floatToRawIntBits(f);
+ int s = (bits >>> FP32_SIGN_SHIFT );
+ int e = (bits >>> FP32_EXPONENT_SHIFT) & FP32_SHIFTED_EXPONENT_MASK;
+ int m = (bits ) & FP32_SIGNIFICAND_MASK;
+ int outE = 0;
+ int outM = 0;
+ if (e == 0xff) { // Infinite or NaN
+ outE = 0x1f;
+ outM = m != 0 ? 0x200 : 0;
+ } else {
+ e = e - FP32_EXPONENT_BIAS + EXPONENT_BIAS;
+ if (e >= 0x1f) { // Overflow
+ outE = 0x1f;
+ } else if (e <= 0) { // Underflow
+ if (e < -10) {
+ // The absolute fp32 value is less than MIN_VALUE, flush to +/-0
+ } else {
+ // The fp32 value is a normalized float less than MIN_NORMAL,
+ // we convert to a denorm fp16
+ m = m | 0x800000;
+ int shift = 14 - e;
+ outM = m >> shift;
+ int lowm = m & ((1 << shift) - 1);
+ int hway = 1 << (shift - 1);
+ // if above halfway or exactly halfway and outM is odd
+ if (lowm + (outM & 1) > hway){
+ // Round to nearest even
+ // Can overflow into exponent bit, which surprisingly is OK.
+ // This increment relies on the +outM in the return statement below
+ outM++;
+ }
+ }
+ } else {
+ outE = e;
+ outM = m >> 13;
+ // if above halfway or exactly halfway and outM is odd
+ if ((m & 0x1fff) + (outM & 0x1) > 0x1000) {
+ // Round to nearest even
+ // Can overflow into exponent bit, which surprisingly is OK.
+ // This increment relies on the +outM in the return statement below
+ outM++;
+ }
+ }
+ }
+ // The outM is added here as the +1 increments for outM above can
+ // cause an overflow in the exponent bit which is OK.
+ return (short) ((s << SIGN_SHIFT) | (outE << EXPONENT_SHIFT) + outM);
+ }
+
+ /**
+ * Returns true if the specified half-precision float value represents
+ * a Not-a-Number, false otherwise.
+ *
+ * @param h A half-precision float value
+ * @return True if the value is a NaN, false otherwise
+ *
+ */
+ public static boolean isNaN(short h) {
+ return (h & EXPONENT_SIGNIFICAND_MASK) > POSITIVE_INFINITY;
+ }
+
+ /**
+ * Returns true if the two half-precision float values are equal.
+ * If either of the values is NaN, the result is false. {@link
#POSITIVE_ZERO}
+ * and {@link #NEGATIVE_ZERO} are considered equal.
+ *
+ * @param x The first half-precision value
+ * @param y The second half-precision value
+ *
+ * @return True if x is equal to y, false otherwise
+ */
+ public static boolean equals(short x, short y) {
+ if (isNaN(x)) return false;
+ if (isNaN(y)) return false;
+ return x == y || ((x | y) & EXPONENT_SIGNIFICAND_MASK) == 0;
+ }
+
+ /**
+ * Returns true if the first half-precision float value is less (smaller
+ * toward negative infinity) than the second half-precision float value.
+ * If either of the values is NaN, the result is false.
+ *
+ * @param x The first half-precision value
+ * @param y The second half-precision value
+ *
+ * @return True if x is less than y, false otherwise
+ */
+ public static boolean less(short x, short y) {
+ if (isNaN(x)) return false;
+ if (isNaN(y)) return false;
+ return ((x & SIGN_MASK) != 0 ? 0x8000 - (x & 0xffff) : x & 0xffff) <
+ ((y & SIGN_MASK) != 0 ? 0x8000 - (y & 0xffff) : y & 0xffff);
+ }
+
+ /**
+ * Returns true if the first half-precision float value is greater (larger
+ * toward positive infinity) than the second half-precision float value.
+ * If either of the values is NaN, the result is false.
+ *
+ * @param x The first half-precision value
+ * @param y The second half-precision value
+ *
+ * @return True if x is greater than y, false otherwise
+ */
+ public static boolean greater(short x, short y) {
+ if (isNaN(x)) return false;
+ if (isNaN(y)) return false;
+ return ((x & SIGN_MASK) != 0 ? 0x8000 - (x & 0xffff) : x & 0xffff) >
+ ((y & SIGN_MASK) != 0 ? 0x8000 - (y & 0xffff) : y & 0xffff);
+ }
+
+ /**
+ * <p>Compares the two specified half-precision float values. The following
+ * conditions apply during the comparison:</p>
+ *
+ * <ul>
+ * <li>{@link #NaN} is considered by this method to be equal to itself and
greater
+ * than all other half-precision float values (including {@code
#POSITIVE_INFINITY})</li>
+ * <li>{@link #POSITIVE_ZERO} is considered by this method to be greater than
+ * {@link #NEGATIVE_ZERO}.</li>
+ * </ul>
+ *
+ * @param x The first half-precision float value to compare.
+ * @param y The second half-precision float value to compare
+ *
+ * @return The value {@code 0} if {@code x} is numerically equal to {@code
y}, a
+ * value less than {@code 0} if {@code x} is numerically less than
{@code y},
+ * and a value greater than {@code 0} if {@code x} is numerically
greater
+ * than {@code y}
+ *
+ */
+ public static int compare(short x, short y) {
+ if (less(x, y)) return -1;
+ if (greater(x, y)) return 1;
+ // Collapse NaNs, akin to halfToIntBits(), but we want to keep
+ // (signed) short value types to preserve the ordering of -0.0
+ // and +0.0
+ short xBits = isNaN(x) ? NaN : x;
+ short yBits = isNaN(y) ? NaN : y;
+ return (xBits == yBits ? 0 : (xBits < yBits ? -1 : 1));
+ }
+
+ /**
+ * Converts byte[] representation in Little-Endian
+ * of a half-precision float value to its half-precision float value.
+ * Throw InvalidFloat16ValueException if length of bytes is not equal to 2.
+ *
+ * @param bytes byte[] representation of a half-precision float value in
Little-Endian
+ * @return A half-precision float value
+ */
+ public static short fromBytesLittleEndian(byte[] bytes) {
+ if (bytes.length != 2) {
+ throw new InvalidFloat16ValueException(String.valueOf(bytes));
Review Comment:
Does `String.valueOf` reflect the length of the input bytes? If not, we
probably need to add it to the error message.
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Float16Statistics.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.parquet.column.statistics;
+
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.apache.parquet.schema.PrimitiveType;
+import org.apache.parquet.schema.Types;
+
+public class Float16Statistics extends BinaryStatistics
+{
+ // A fake type object to be used to generate the proper comparator
+ private static final PrimitiveType DEFAULT_FAKE_TYPE =
Types.optional(PrimitiveType.PrimitiveTypeName.BINARY)
+
.named("fake_binary_float16_type").withLogicalTypeAnnotation(LogicalTypeAnnotation.float16Type());
Review Comment:
```suggestion
private static final PrimitiveType DEFAULT_FAKE_TYPE =
Types.optional(PrimitiveType.PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)
.named("fake_float16_type").withLogicalTypeAnnotation(LogicalTypeAnnotation.float16Type());
```
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Float16Statistics.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.parquet.column.statistics;
+
+import org.apache.parquet.schema.LogicalTypeAnnotation;
+import org.apache.parquet.schema.PrimitiveType;
+import org.apache.parquet.schema.Types;
+
+public class Float16Statistics extends BinaryStatistics
+{
Review Comment:
```suggestion
public class Float16Statistics extends BinaryStatistics {
```
##########
parquet-common/src/main/java/org/apache/parquet/type/Float16.java:
##########
@@ -0,0 +1,339 @@
+/**
+ * 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
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.type;
+
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+
+/**
+ * The class is a utility class to manipulate half-precision 16-bit
+ * <a
href="https://en.wikipedia.org/wiki/Half-precision_floating-point_format";>IEEE
754</a>
+ * floating point data types (also called fp16 or binary16). A half-precision
float can be
+ * created from or converted to single-precision floats, and is stored in a
short data type.
+ * The IEEE 754 standard specifies an float16 as having the following format:
+ * <ul>
+ * <li>Sign bit: 1 bit</li>
+ * <li>Exponent width: 5 bits</li>
+ * <li>Significand: 10 bits</li>
+ * </ul>
+ *
+ * <p>The format is laid out as follows:</p>
+ * <pre>
+ * 1 11111 1111111111
+ * ^ --^-- -----^----
+ * sign | |_______ significand
+ * |
+ * -- exponent
+ * </pre>
+ * Half-precision floating points can be useful to save memory and/or
+ * bandwidth at the expense of range and precision when compared to
single-precision
+ * floating points (float32).
+ * Ref:
https://android.googlesource.com/platform/libcore/+/master/luni/src/main/java/libcore/util/FP16.java
+ */
+public class Float16
+{
Review Comment:
```suggestion
public class Float16 {
```
##########
parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java:
##########
@@ -276,4 +278,22 @@ public String toString() {
return "BINARY_AS_SIGNED_INTEGER_COMPARATOR";
}
};
+
+ /**
+ * This comparator is for comparing two float16 values represented in 2
bytes binary.
+ */
+ static final PrimitiveComparator<Binary> BINARY_AS_FLOAT16_COMPARATOR = new
BinaryComparator() {
+
+ @Override
+ int compareBinary(Binary b1, Binary b2)
+ {
Review Comment:
```suggestion
int compareBinary(Binary b1, Binary b2) {
```
##########
parquet-column/src/main/java/org/apache/parquet/column/statistics/Statistics.java:
##########
@@ -139,6 +140,43 @@ public Statistics<?> build() {
}
}
+ // Builder for FLOAT16 type to handle special cases of min/max values like
NaN, -0.0, and 0.0
+ private static class Float16Builder extends Builder {
+ public Float16Builder(PrimitiveType type) {
+ super(type);
+ assert type.getPrimitiveTypeName() == PrimitiveTypeName.BINARY;
+ }
+
+ @Override
+ public Statistics<?> build() {
Review Comment:
@benibus Could you please double check if this aligns with the C++ impl?
##########
parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java:
##########
@@ -261,6 +260,11 @@ public Optional<PrimitiveComparator>
visit(LogicalTypeAnnotation.JsonLogicalType
public Optional<PrimitiveComparator>
visit(LogicalTypeAnnotation.BsonLogicalTypeAnnotation bsonLogicalType) {
return
of(PrimitiveComparator.UNSIGNED_LEXICOGRAPHICAL_BINARY_COMPARATOR);
}
+
+ @Override
+ public Optional<PrimitiveComparator>
visit(LogicalTypeAnnotation.Float16LogicalTypeAnnotation float16LogicalType) {
+ return of(PrimitiveComparator.BINARY_AS_FLOAT16_COMPARATOR);
+ }
Review Comment:
```suggestion
```
##########
parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java:
##########
@@ -268,6 +271,36 @@ public void
testBinaryAsSignedIntegerComparatorWithEquals() {
}
}
+ @Test
+ public void testFloat16Comparator() {
+ short[] valuesInAscendingOrder = {
+ (short) 0xfc00,
+ Float16.MIN_VALUE,
+ -Float16.MAX_VALUE,
+ (short) 0xc000,
+ -Float16.MIN_VALUE,
Review Comment:
Do you mean `-Float16.MIN_VALUE < 0 < Float16.MIN_VALUE`? This seems
incorrect.
##########
parquet-common/src/test/java/org/apache/parquet/type/TestFloat16.java:
##########
@@ -0,0 +1,244 @@
+/*
+ * 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
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.parquet.type;
+
+import org.junit.Test;
+
+import static org.apache.parquet.type.Float16.MIN_NORMAL;
+import static org.junit.Assert.assertEquals;
+import static org.apache.parquet.type.Float16.toFloat16;
+import static org.apache.parquet.type.Float16.toFloat;
+import static org.apache.parquet.type.Float16.POSITIVE_INFINITY;
+import static org.apache.parquet.type.Float16.NEGATIVE_INFINITY;
+import static org.apache.parquet.type.Float16.MIN_VALUE;
+import static org.apache.parquet.type.Float16.LOWEST_VALUE;
+import static org.apache.parquet.type.Float16.MAX_VALUE;
+import static org.apache.parquet.type.Float16.POSITIVE_ZERO;
+import static org.apache.parquet.type.Float16.NEGATIVE_ZERO;
+import static org.apache.parquet.type.Float16.NaN;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertFalse;
+
+public class TestFloat16
+{
Review Comment:
```suggestion
public class TestFloat16 {
```
##########
parquet-common/src/main/java/org/apache/parquet/type/InvalidFloat16ValueException.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.type;
+
+import org.apache.parquet.ParquetRuntimeException;
+
+/**
+ * Thrown if Binary is invalid as a Float16 value.
+ */
+public class InvalidFloat16ValueException extends ParquetRuntimeException
+{
Review Comment:
```suggestion
public class InvalidFloat16ValueException extends ParquetRuntimeException {
```
##########
parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java:
##########
@@ -268,6 +271,36 @@ public void
testBinaryAsSignedIntegerComparatorWithEquals() {
}
}
+ @Test
+ public void testFloat16Comparator() {
+ short[] valuesInAscendingOrder = {
+ (short) 0xfc00,
+ Float16.MIN_VALUE,
+ -Float16.MAX_VALUE,
+ (short) 0xc000,
+ -Float16.MIN_VALUE,
+ 0,
+ Float16.MIN_VALUE,
+ (short) 0x7bff,
+ Float16.MAX_VALUE,
+ (short) 0x7c00};
+
+ for (int i = 0; i < valuesInAscendingOrder.length; ++i) {
+ for (int j = 0; j < valuesInAscendingOrder.length; ++j) {
+ short vi = valuesInAscendingOrder[i];
+ short vj = valuesInAscendingOrder[j];
+ ByteBuffer bbi = ByteBuffer.allocate(2).order(ByteOrder.LITTLE_ENDIAN);
+ bbi.putShort(vi).flip();
+ ByteBuffer bbj = ByteBuffer.allocate(2).order(ByteOrder.LITTLE_ENDIAN);
+ bbj.putShort(vj).flip();
+ float fi = Float16.toFloat(vi);
+ float fj = Float16.toFloat(vj);
+ assertEquals(Float.compare(fi, fj),
BINARY_AS_FLOAT16_COMPARATOR.compare(
Review Comment:
If values are in the ascending order, we should also verify that if i < j,
then fi < fj.
##########
parquet-column/src/test/java/org/apache/parquet/schema/TestPrimitiveComparator.java:
##########
@@ -268,6 +271,36 @@ public void
testBinaryAsSignedIntegerComparatorWithEquals() {
}
}
+ @Test
+ public void testFloat16Comparator() {
+ short[] valuesInAscendingOrder = {
+ (short) 0xfc00,
+ Float16.MIN_VALUE,
+ -Float16.MAX_VALUE,
+ (short) 0xc000,
+ -Float16.MIN_VALUE,
+ 0,
Review Comment:
Add +0 and -0 as well?
##########
parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java:
##########
@@ -261,6 +260,11 @@ public Optional<PrimitiveComparator>
visit(LogicalTypeAnnotation.JsonLogicalType
public Optional<PrimitiveComparator>
visit(LogicalTypeAnnotation.BsonLogicalTypeAnnotation bsonLogicalType) {
return
of(PrimitiveComparator.UNSIGNED_LEXICOGRAPHICAL_BINARY_COMPARATOR);
}
+
+ @Override
+ public Optional<PrimitiveComparator>
visit(LogicalTypeAnnotation.Float16LogicalTypeAnnotation float16LogicalType) {
+ return of(PrimitiveComparator.BINARY_AS_FLOAT16_COMPARATOR);
+ }
Review Comment:
These lines should be removed. They only apply to FIXED_LENGTH_BYTE_ARRAY.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@parquet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
