switched to signed binary comparison, added tests

Author: Artem Tarasov

src/parquet/util/CMakeLists.txt

@@ -71,6 +71,7 @@ endif()
 
 ADD_PARQUET_TEST(bit-util-test)
 ADD_PARQUET_TEST(buffer-test)
+ADD_PARQUET_TEST(comparison-test)
 ADD_PARQUET_TEST(input-output-test)
 ADD_PARQUET_TEST(mem-allocator-test)
 ADD_PARQUET_TEST(mem-pool-test)

src/parquet/util/comparison-test.cc

@@ -0,0 +1,91 @@
+// 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.
+
+#include <gtest/gtest.h>
+
+#include "parquet/schema/descriptor.h"
+#include "parquet/types.h"
+#include "parquet/util/comparison.h"
+
+#include <cstdint>
+#include <iostream>
+#include <vector>
+
+namespace parquet {
+
+namespace test {
+
+static ByteArray ByteArrayFromString(const std::string& s) {
+  auto ptr = reinterpret_cast<const uint8_t*>(s.data());
+  return ByteArray(s.size(), ptr);
+}
+
+static FLBA FLBAFromString(const std::string& s) {
+  auto ptr = reinterpret_cast<const uint8_t*>(s.data());
+  return FLBA(ptr);
+}
+
+TEST(Comparison, ByteArray) {
+  using namespace parquet::schema;
+  NodePtr node = PrimitiveNode::Make("bytearray", Repetition::REQUIRED, Type::BYTE_ARRAY);
+  ColumnDescriptor descr(node, 0, 0);
+  Compare<parquet::ByteArray> less(&descr);
+
+  std::string a = "arrange";
+  std::string b = "arrangement";
+  auto arr1 = ByteArrayFromString(a);
+  auto arr2 = ByteArrayFromString(b);
+  ASSERT_TRUE(less(arr1, arr2));
+
+  a = u8"braten";
+  b = u8"bügeln";
+  auto arr3 = ByteArrayFromString(a);
+  auto arr4 = ByteArrayFromString(b);
+  // see PARQUET-686 discussion about binary comparison
+  ASSERT_TRUE(!less(arr3, arr4));
+}
+
+TEST(Comparison, FLBA) {
+  using namespace parquet::schema;
+
+  std::string a = "Antidisestablishmentarianism";
+  std::string b = "Bundesgesundheitsministerium";
+  auto arr1 = FLBAFromString(a);
+  auto arr2 = FLBAFromString(b);
+
+  NodePtr node = PrimitiveNode::Make("FLBA", Repetition::REQUIRED,
+      Type::FIXED_LEN_BYTE_ARRAY, LogicalType::NONE, a.size());
+  ColumnDescriptor descr(node, 0, 0);
+  Compare<parquet::FixedLenByteArray> less(&descr);
+  ASSERT_TRUE(less(arr1, arr2));
+}
+
+TEST(Comparison, Int96) {
+  parquet::Int96 a{1, 41, 14}, b{1, 41, 42};
+
+  using namespace parquet::schema;
+  NodePtr node = PrimitiveNode::Make("int96", Repetition::REQUIRED, Type::INT96);
+  ColumnDescriptor descr(node, 0, 0);
+  Compare<parquet::Int96> less(&descr);
+  ASSERT_TRUE(less(a, b));
+  b.value[2] = 14;
+  ASSERT_TRUE(!less(a, b) && !less(b, a));
+}
+
+}  // namespace test
+
+}  // namespace parquet

src/parquet/util/comparison.h

@@ -42,13 +42,17 @@ inline bool Compare<Int96>::operator()(const Int96& a, const Int96& b) {
 
 template <>
 inline bool Compare<ByteArray>::operator()(const ByteArray& a, const ByteArray& b) {
-  return std::lexicographical_compare(a.ptr, a.ptr + a.len, b.ptr, b.ptr + b.len);
+  auto aptr = reinterpret_cast<const int8_t*>(a.ptr);
+  auto bptr = reinterpret_cast<const int8_t*>(b.ptr);
+  return std::lexicographical_compare(aptr, aptr + a.len, bptr, bptr + b.len);
 }
 
 template <>
 inline bool Compare<FLBA>::operator()(const FLBA& a, const FLBA& b) {
+  auto aptr = reinterpret_cast<const int8_t*>(a.ptr);
+  auto bptr = reinterpret_cast<const int8_t*>(b.ptr);
   return std::lexicographical_compare(
-      a.ptr, a.ptr + type_length_, b.ptr, b.ptr + type_length_);
+      aptr, aptr + type_length_, bptr, bptr + type_length_);
 }
 
 }  // namespace parquet