You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2024/07/09 06:29:49 UTC
[PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
paleolimbot opened a new pull request, #43196:
URL: https://github.com/apache/arrow/pull/43196
### Rationale for this change
In https://github.com/apache/parquet-format/pull/240 a GEOMETRY logical type for Parquet is proposed with a proof-of-concept Java implementation ( https://github.com/apache/parquet-java/pull/1379 ). This is a PR to explore what an implementation would look like in C++.
### What changes are included in this PR?
Work in progress!
### Are these changes tested?
They will be!
### Are there any user-facing changes?
Yes! (And will eventually be documented)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #43196:
URL: https://github.com/apache/arrow/pull/43196#issuecomment-2322743110
@paleolimbot Hey Dewey, I know you have been busy at other projects. We really want to speed up the process of the Parquet Geometry PR since the proposal and Java POC is already done. Do you have an estimated timeline for the C++ POC implementation?
If you have too much on your plate, we will be happy to take over or help with the C++ POC.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1745867358
##########
cpp/src/parquet/statistics.h:
##########
@@ -372,11 +435,14 @@ template <typename DType>
std::shared_ptr<TypedStatistics<DType>> MakeStatistics(
const ColumnDescriptor* descr, const std::string& encoded_min,
const std::string& encoded_max, int64_t num_values, int64_t null_count,
- int64_t distinct_count, bool has_min_max, bool has_null_count,
- bool has_distinct_count, ::arrow::MemoryPool* pool = ::arrow::default_memory_pool()) {
- return std::static_pointer_cast<TypedStatistics<DType>>(Statistics::Make(
- descr, encoded_min, encoded_max, num_values, null_count, distinct_count,
- has_min_max, has_null_count, has_distinct_count, pool));
+ int64_t distinct_count, const EncodedGeometryStatistics& geometry_statistics,
Review Comment:
Would adding the two new arguments to the end of the argument list (with default values) make this a non-breaking change?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1745799535
##########
cpp/src/parquet/statistics.cc:
##########
@@ -47,6 +48,155 @@ using arrow::util::SafeCopy;
using arrow::util::SafeLoad;
namespace parquet {
+
+class GeometryStatisticsImpl {
+ public:
+ bool Equals(const GeometryStatisticsImpl& other) const {
+ if (is_valid_ != other.is_valid_) {
+ return false;
+ }
+
+ if (!is_valid_ && !other.is_valid_) {
+ return true;
+ }
+
+ auto wkb_types = bounder_.WkbTypes();
+ auto other_wkb_types = other.bounder_.WkbTypes();
+ if (wkb_types.size() != other_wkb_types.size()) {
+ return false;
+ }
+
+ for (size_t i = 0; i < wkb_types.size(); i++) {
+ if (wkb_types[i] != other_wkb_types[i]) {
+ return false;
+ }
+ }
+
+ return bounder_.Bounds() == other.bounder_.Bounds();
+ }
+
+ void Merge(const GeometryStatisticsImpl& other) {
+ if (!is_valid_ || !other.is_valid_) {
+ is_valid_ = false;
+ return;
+ }
+
+ bounder_.ReadBox(other.bounder_.Bounds());
+ bounder_.ReadGeometryTypes(other.bounder_.WkbTypes());
+ }
+
+ void Update(const ByteArray* values, int64_t num_values, int64_t null_count) {
Review Comment:
We might need a `UpdateSpaced` variant which deals with inputs with validity bits, like what `TypeStatistics::UpdateSpaced` does.
##########
cpp/src/parquet/statistics.h:
##########
@@ -135,6 +186,7 @@ class PARQUET_EXPORT EncodedStatistics {
bool has_max = false;
bool has_null_count = false;
bool has_distinct_count = false;
+ bool has_geometry_statistics = false;
Review Comment:
I know other fields already have `has_xxx` flags before C++11. Should we use `std::optional<EncodedGeometryStatistics>` to make it simpler?
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
Review Comment:
Should we throw when edges is UNKNOWN? It is a required field by design.
##########
cpp/src/parquet/geometry_util.h:
##########
@@ -0,0 +1,614 @@
+// 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.
+
+#pragma once
+
+#include <algorithm>
+#include <limits>
+#include <string>
+#include <unordered_set>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/ubsan.h"
+#include "parquet/exception.h"
+
+namespace parquet::geometry {
+
+constexpr double kInf = std::numeric_limits<double>::infinity();
+
+struct Dimensions {
+ enum dimensions { XY = 0, XYZ = 1, XYM = 2, XYZM = 3 };
+
+ static dimensions FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type / 1000) {
+ case 0:
+ return XY;
+ case 1:
+ return XYZ;
+ case 2:
+ return XYM;
+ case 3:
+ return XYZM;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ template <dimensions dims>
+ constexpr static uint32_t size();
+
+ template <>
+ constexpr uint32_t size<XY>() {
+ return 2;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZ>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYM>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZM>() {
+ return 4;
+ }
+
+ static uint32_t size(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return size<XY>();
+ case XYZ:
+ return size<XYZ>();
+ case XYM:
+ return size<XYM>();
+ case XYZM:
+ return size<XYZM>();
+ default:
+ return 0;
+ }
+ }
+
+ // Where to look in a coordinate with this dimension
+ // for the X, Y, Z, and M dimensions, respectively.
+ static std::array<int, 4> ToXYZM(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return {0, 1, -1, -1};
+ case XYZ:
+ return {0, 1, 2, -1};
+ case XYM:
+ return {0, 1, -1, 2};
+ case XYZM:
+ return {0, 1, 2, 3};
+ default:
+ return {-1, -1, -1, -1};
+ }
+ }
+
+ static std::string ToString(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return "XY";
+ case XYZ:
+ return "XYZ";
+ case XYM:
+ return "XYM";
+ case XYZM:
+ return "XYZM";
+ default:
+ return "";
+ }
+ }
+};
+
+struct GeometryType {
+ enum geometry_type {
+ POINT = 1,
+ LINESTRING = 2,
+ POLYGON = 3,
+ MULTIPOINT = 4,
+ MULTILINESTRING = 5,
+ MULTIPOLYGON = 6,
+ GEOMETRYCOLLECTION = 7
+ };
+
+ static geometry_type FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type % 1000) {
+ case 1:
+ return POINT;
+ case 2:
+ return LINESTRING;
+ case 3:
+ return POLYGON;
+ case 4:
+ return MULTIPOINT;
+ case 5:
+ return MULTILINESTRING;
+ case 6:
+ return MULTIPOLYGON;
+ case 7:
+ return GEOMETRYCOLLECTION;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ static std::string ToString(geometry_type geometry_type) {
+ switch (geometry_type) {
+ case POINT:
+ return "POINT";
+ case LINESTRING:
+ return "LINESTRING";
+ case POLYGON:
+ return "POLYGON";
+ case MULTIPOINT:
+ return "MULTIPOINT";
+ case MULTILINESTRING:
+ return "MULTILINESTRING";
+ case MULTIPOLYGON:
+ return "MULTIPOLYGON";
+ case GEOMETRYCOLLECTION:
+ return "GEOMETRYCOLLECTION";
+ default:
+ return "";
+ }
+ }
+};
+
+class WKBBuffer {
+ public:
+ WKBBuffer() : data_(nullptr), size_(0) {}
+ WKBBuffer(const uint8_t* data, int64_t size) : data_(data), size_(size) {}
+
+ void Init(const uint8_t* data, int64_t size) {
+ data_ = data;
+ size_ = size;
+ }
+
+ uint8_t ReadUInt8() {
+ if (size_ < 1) {
+ throw ParquetException("Can't read 1 byte from empty WKBBuffer");
+ }
+
+ size_ -= 1;
+ return *data_++;
+ }
+
+ uint32_t ReadUInt32(bool swap) {
+ if (ARROW_PREDICT_FALSE(swap)) {
+ return ReadUInt32<true>();
+ } else {
+ return ReadUInt32<false>();
+ }
+ }
+
+ template <bool swap>
+ uint32_t ReadUInt32() {
+ if (size_ < sizeof(uint32_t)) {
+ throw ParquetException("Can't read 4 bytes from WKBBuffer with ", size_,
+ "remaining");
+ }
+
+ uint32_t value;
+ memcpy(&value, data_, sizeof(uint32_t));
+ data_ += sizeof(uint32_t);
+ size_ -= sizeof(uint32_t);
+
+ if constexpr (swap) {
+ value = ::arrow::bit_util::ByteSwap(value);
+ }
+
+ return value;
+ }
+
+ template <bool swap>
+ void ReadDoubles(uint32_t n, double* out) {
+ if (n == 0) {
+ return;
+ }
+
+ size_t total_bytes = n * sizeof(double);
+ if (size_ < total_bytes) {
+ throw ParquetException("Can't read ", total_bytes, " bytes from WKBBuffer with ",
+ size_, "remaining");
+ }
+
+ memcpy(out, data_, total_bytes);
+ data_ += total_bytes;
+ size_ -= total_bytes;
+
+ if constexpr (swap) {
+ for (uint32_t i = 0; i < n; i++) {
+ out[i] = ::arrow::bit_util::ByteSwap(out[i]);
+ }
+ }
+ }
+
+ size_t size() { return size_; }
+
+ private:
+ const uint8_t* data_;
+ size_t size_;
+};
+
+struct BoundingBox {
+ BoundingBox(Dimensions::dimensions dimensions, const std::array<double, 4>& mins,
+ const std::array<double, 4>& maxes)
+ : dimensions(dimensions) {
+ std::memcpy(min, mins.data(), sizeof(min));
+ std::memcpy(max, maxes.data(), sizeof(max));
+ }
+ explicit BoundingBox(Dimensions::dimensions dimensions = Dimensions::XYZM)
+ : dimensions(dimensions),
+ min{kInf, kInf, kInf, kInf},
+ max{-kInf, -kInf, -kInf, -kInf} {}
+
+ BoundingBox(const BoundingBox& other) = default;
+ BoundingBox& operator=(const BoundingBox&) = default;
+
+ void Reset() {
+ for (int i = 0; i < 4; i++) {
+ min[i] = kInf;
+ max[i] = -kInf;
+ }
+ }
+
+ void Merge(const BoundingBox& other) {
+ if (ARROW_PREDICT_TRUE(dimensions == other.dimensions)) {
+ for (int i = 0; i < 4; i++) {
+ min[i] = std::min(min[i], other.min[i]);
+ max[i] = std::max(max[i], other.max[i]);
+ }
+
+ return;
+ } else if (dimensions == Dimensions::XYZM) {
+ Merge(other.ToXYZM());
+ } else {
+ ParquetException::NYI();
+ }
+ }
+
+ BoundingBox ToXYZM() const {
+ BoundingBox xyzm(Dimensions::XYZM);
+ auto to_xyzm = Dimensions::ToXYZM(dimensions);
+ for (int i = 0; i < 4; i++) {
+ int dim_to_xyzm = to_xyzm[i];
+ if (dim_to_xyzm == -1) {
+ xyzm.min[i] = kInf;
+ xyzm.max[i] = -kInf;
+ } else {
+ xyzm.min[i] = min[dim_to_xyzm];
+ xyzm.max[i] = max[dim_to_xyzm];
+ }
+ }
+
+ return xyzm;
+ }
+
+ std::string ToString() const {
+ std::stringstream ss;
+ ss << "BoundingBox " << Dimensions::ToString(dimensions) << " [" << min[0] << " => "
+ << max[0];
+ for (int i = 1; i < 4; i++) {
+ ss << ", " << min[i] << " => " << max[i];
+ }
+
+ ss << "]";
+
+ return ss.str();
+ }
+
+ Dimensions::dimensions dimensions;
+ double min[4];
+ double max[4];
+};
+
+inline bool operator==(const BoundingBox& lhs, const BoundingBox& rhs) {
+ return lhs.dimensions == rhs.dimensions &&
+ std::memcmp(lhs.min, rhs.min, sizeof(lhs.min)) == 0 &&
+ std::memcmp(lhs.max, rhs.max, sizeof(lhs.max)) == 0;
+}
+
+template <Dimensions::dimensions dims, bool swap, uint32_t chunk_size>
+class WKBSequenceBounder {
+ public:
+ explicit WKBSequenceBounder(double* chunk) : box_(dims), chunk_(chunk) {}
+
+ void ReadPoint(WKBBuffer* src) {
+ constexpr uint32_t coord_size = Dimensions::size<dims>();
+ src->ReadDoubles<swap>(coord_size, chunk_);
+ for (uint32_t dim = 0; dim < coord_size; dim++) {
+ if (ARROW_PREDICT_TRUE(!std::isnan(chunk_[dim]))) {
+ box_.min[dim] = std::min(box_.min[dim], chunk_[dim]);
+ box_.max[dim] = std::max(box_.max[dim], chunk_[dim]);
+ }
+ }
+ }
+
+ void ReadSequence(WKBBuffer* src) {
+ constexpr uint32_t coord_size = Dimensions::size<dims>();
+ constexpr uint32_t coords_per_chunk = chunk_size / coord_size;
+
+ uint32_t n_coords = src->ReadUInt32<swap>();
+ uint32_t n_chunks = n_coords / coords_per_chunk;
+ for (uint32_t i = 0; i < n_chunks; i++) {
+ src->ReadDoubles<swap>(coords_per_chunk, chunk_);
+ ReadChunk(coords_per_chunk);
+ }
+
+ uint32_t remaining_coords = n_coords - (n_chunks * coords_per_chunk);
+ src->ReadDoubles<swap>(remaining_coords * coord_size, chunk_);
+ ReadChunk(remaining_coords);
+ }
+
+ void ReadRings(WKBBuffer* src) {
+ uint32_t n_rings = src->ReadUInt32<swap>();
+ for (uint32_t i = 0; i < n_rings; i++) {
+ ReadSequence(src);
+ }
+ }
+
+ void Reset() { box_.Reset(); }
+
+ void Finish(BoundingBox* out) { out->Merge(box_); }
+
+ private:
+ BoundingBox box_;
+ double* chunk_;
+
+ void ReadChunk(uint32_t n_coords) {
+ constexpr uint32_t coord_size = Dimensions::size<dims>();
+ for (uint32_t dim = 0; dim < coord_size; dim++) {
+ for (uint32_t i = 0; i < n_coords; i++) {
+ box_.min[dim] = std::min(box_.min[dim], chunk_[i * coord_size + dim]);
+ box_.max[dim] = std::max(box_.max[dim], chunk_[i * coord_size + dim]);
+ }
+ }
+ }
+};
+
+// We could (should?) avoid this madness by not templating the WKBSequenceBounder
+class WKBGenericSequenceBounder {
+ public:
+ WKBGenericSequenceBounder()
+ : xy_(chunk_),
Review Comment:
Why do we need to keep 8 variants?
##########
cpp/src/parquet/geometry_util.h:
##########
@@ -0,0 +1,603 @@
+// 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.
+
+#pragma once
+
+#include <algorithm>
+#include <limits>
+#include <string>
+#include <unordered_set>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/ubsan.h"
+#include "parquet/exception.h"
+
+namespace parquet::geometry {
+
+constexpr double kInf = std::numeric_limits<double>::infinity();
+
+struct Dimensions {
+ enum dimensions { XY = 0, XYZ = 1, XYM = 2, XYZM = 3 };
+
+ static dimensions FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type / 1000) {
+ case 0:
+ return XY;
+ case 1:
+ return XYZ;
+ case 2:
+ return XYM;
+ case 3:
+ return XYZM;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ template <dimensions dims>
+ constexpr static uint32_t size();
+
+ template <>
+ constexpr uint32_t size<XY>() {
+ return 2;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZ>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYM>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZM>() {
+ return 4;
+ }
+
+ static uint32_t size(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return size<XY>();
+ case XYZ:
+ return size<XYZ>();
+ case XYM:
+ return size<XYM>();
+ case XYZM:
+ return size<XYZM>();
+ default:
+ return 0;
+ }
+ }
+
+ // Where to look in a coordinate with this dimension
+ // for the X, Y, Z, and M dimensions, respectively.
+ static std::array<int, 4> ToXYZM(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return {0, 1, -1, -1};
+ case XYZ:
+ return {0, 1, 2, -1};
+ case XYM:
+ return {0, 1, -1, 2};
+ case XYZM:
+ return {0, 1, 2, 3};
+ default:
+ return {-1, -1, -1, -1};
+ }
+ }
+
+ static std::string ToString(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return "XY";
+ case XYZ:
+ return "XYZ";
+ case XYM:
+ return "XYM";
+ case XYZM:
+ return "XYZM";
+ default:
+ return "";
+ }
+ }
+};
+
+struct GeometryType {
+ enum geometry_type {
+ POINT = 1,
+ LINESTRING = 2,
+ POLYGON = 3,
+ MULTIPOINT = 4,
+ MULTILINESTRING = 5,
+ MULTIPOLYGON = 6,
+ GEOMETRYCOLLECTION = 7
+ };
+
+ static geometry_type FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type % 1000) {
+ case 1:
+ return POINT;
+ case 2:
+ return LINESTRING;
+ case 3:
+ return POLYGON;
+ case 4:
+ return MULTIPOINT;
+ case 5:
+ return MULTILINESTRING;
+ case 6:
+ return MULTIPOLYGON;
+ case 7:
+ return GEOMETRYCOLLECTION;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ static std::string ToString(geometry_type geometry_type) {
+ switch (geometry_type) {
+ case POINT:
+ return "POINT";
+ case LINESTRING:
+ return "LINESTRING";
+ case POLYGON:
+ return "POLYGON";
+ case MULTIPOINT:
+ return "MULTIPOINT";
+ case MULTILINESTRING:
+ return "MULTILINESTRING";
+ case MULTIPOLYGON:
+ return "MULTIPOLYGON";
+ case GEOMETRYCOLLECTION:
+ return "GEOMETRYCOLLECTION";
+ default:
+ return "";
+ }
+ }
+};
+
+class WKBBuffer {
Review Comment:
After thinking twice, I think we can keep this in the parquet module. But it should be renamed to `geometry_util_internal.h` to avoid making it public.
##########
cpp/src/parquet/statistics.h:
##########
@@ -372,11 +435,14 @@ template <typename DType>
std::shared_ptr<TypedStatistics<DType>> MakeStatistics(
const ColumnDescriptor* descr, const std::string& encoded_min,
const std::string& encoded_max, int64_t num_values, int64_t null_count,
- int64_t distinct_count, bool has_min_max, bool has_null_count,
- bool has_distinct_count, ::arrow::MemoryPool* pool = ::arrow::default_memory_pool()) {
- return std::static_pointer_cast<TypedStatistics<DType>>(Statistics::Make(
- descr, encoded_min, encoded_max, num_values, null_count, distinct_count,
- has_min_max, has_null_count, has_distinct_count, pool));
+ int64_t distinct_count, const EncodedGeometryStatistics& geometry_statistics,
Review Comment:
Unfortunately this will break downstream users as it is a public API.
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
+ if (type.GEOMETRY.edges == format::Edges::PLANAR) {
+ edges = LogicalType::GeometryEdges::PLANAR;
+ } else if (type.GEOMETRY.edges == format::Edges::SPHERICAL) {
+ edges = LogicalType::GeometryEdges::SPHERICAL;
+ }
+
+ LogicalType::GeometryEncoding::geometry_encoding encoding =
+ LogicalType::GeometryEncoding::UNKNOWN;
Review Comment:
ditto, this is a required field by design.
##########
cpp/src/parquet/types.h:
##########
@@ -166,6 +167,14 @@ class PARQUET_EXPORT LogicalType {
enum unit { UNKNOWN = 0, MILLIS = 1, MICROS, NANOS };
};
+ struct GeometryEncoding {
Review Comment:
Yes, they are workarounds before C++11.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #43196:
URL: https://github.com/apache/arrow/pull/43196#issuecomment-2347247242
Closing in favour of https://github.com/apache/arrow/pull/43977 !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43196:
URL: https://github.com/apache/arrow/pull/43196#issuecomment-2332125711
Thanks @paleolimbot for initiating the PoC! I've left some inline comments on this PR.
Thanks @jiayuasu and @Kontinuation to continue the effort! Let's work together to make it to the finish line.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot closed pull request #43196: WIP: Proof-of-concept Parquet GEOMETRY logical type implementation
URL: https://github.com/apache/arrow/pull/43196
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1745866038
##########
cpp/src/parquet/geometry_util.h:
##########
@@ -0,0 +1,614 @@
+// 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.
+
+#pragma once
+
+#include <algorithm>
+#include <limits>
+#include <string>
+#include <unordered_set>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/ubsan.h"
+#include "parquet/exception.h"
+
+namespace parquet::geometry {
+
+constexpr double kInf = std::numeric_limits<double>::infinity();
+
+struct Dimensions {
+ enum dimensions { XY = 0, XYZ = 1, XYM = 2, XYZM = 3 };
+
+ static dimensions FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type / 1000) {
+ case 0:
+ return XY;
+ case 1:
+ return XYZ;
+ case 2:
+ return XYM;
+ case 3:
+ return XYZM;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ template <dimensions dims>
+ constexpr static uint32_t size();
+
+ template <>
+ constexpr uint32_t size<XY>() {
+ return 2;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZ>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYM>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZM>() {
+ return 4;
+ }
+
+ static uint32_t size(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return size<XY>();
+ case XYZ:
+ return size<XYZ>();
+ case XYM:
+ return size<XYM>();
+ case XYZM:
+ return size<XYZM>();
+ default:
+ return 0;
+ }
+ }
+
+ // Where to look in a coordinate with this dimension
+ // for the X, Y, Z, and M dimensions, respectively.
+ static std::array<int, 4> ToXYZM(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return {0, 1, -1, -1};
+ case XYZ:
+ return {0, 1, 2, -1};
+ case XYM:
+ return {0, 1, -1, 2};
+ case XYZM:
+ return {0, 1, 2, 3};
+ default:
+ return {-1, -1, -1, -1};
+ }
+ }
+
+ static std::string ToString(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return "XY";
+ case XYZ:
+ return "XYZ";
+ case XYM:
+ return "XYM";
+ case XYZM:
+ return "XYZM";
+ default:
+ return "";
+ }
+ }
+};
+
+struct GeometryType {
+ enum geometry_type {
+ POINT = 1,
+ LINESTRING = 2,
+ POLYGON = 3,
+ MULTIPOINT = 4,
+ MULTILINESTRING = 5,
+ MULTIPOLYGON = 6,
+ GEOMETRYCOLLECTION = 7
+ };
+
+ static geometry_type FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type % 1000) {
+ case 1:
+ return POINT;
+ case 2:
+ return LINESTRING;
+ case 3:
+ return POLYGON;
+ case 4:
+ return MULTIPOINT;
+ case 5:
+ return MULTILINESTRING;
+ case 6:
+ return MULTIPOLYGON;
+ case 7:
+ return GEOMETRYCOLLECTION;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ static std::string ToString(geometry_type geometry_type) {
+ switch (geometry_type) {
+ case POINT:
+ return "POINT";
+ case LINESTRING:
+ return "LINESTRING";
+ case POLYGON:
+ return "POLYGON";
+ case MULTIPOINT:
+ return "MULTIPOINT";
+ case MULTILINESTRING:
+ return "MULTILINESTRING";
+ case MULTIPOLYGON:
+ return "MULTIPOLYGON";
+ case GEOMETRYCOLLECTION:
+ return "GEOMETRYCOLLECTION";
+ default:
+ return "";
+ }
+ }
+};
+
+class WKBBuffer {
+ public:
+ WKBBuffer() : data_(nullptr), size_(0) {}
+ WKBBuffer(const uint8_t* data, int64_t size) : data_(data), size_(size) {}
+
+ void Init(const uint8_t* data, int64_t size) {
+ data_ = data;
+ size_ = size;
+ }
+
+ uint8_t ReadUInt8() {
+ if (size_ < 1) {
+ throw ParquetException("Can't read 1 byte from empty WKBBuffer");
+ }
+
+ size_ -= 1;
+ return *data_++;
+ }
+
+ uint32_t ReadUInt32(bool swap) {
+ if (ARROW_PREDICT_FALSE(swap)) {
+ return ReadUInt32<true>();
+ } else {
+ return ReadUInt32<false>();
+ }
+ }
+
+ template <bool swap>
+ uint32_t ReadUInt32() {
+ if (size_ < sizeof(uint32_t)) {
+ throw ParquetException("Can't read 4 bytes from WKBBuffer with ", size_,
+ "remaining");
+ }
+
+ uint32_t value;
+ memcpy(&value, data_, sizeof(uint32_t));
+ data_ += sizeof(uint32_t);
+ size_ -= sizeof(uint32_t);
+
+ if constexpr (swap) {
+ value = ::arrow::bit_util::ByteSwap(value);
+ }
+
+ return value;
+ }
+
+ template <bool swap>
+ void ReadDoubles(uint32_t n, double* out) {
+ if (n == 0) {
+ return;
+ }
+
+ size_t total_bytes = n * sizeof(double);
+ if (size_ < total_bytes) {
+ throw ParquetException("Can't read ", total_bytes, " bytes from WKBBuffer with ",
+ size_, "remaining");
+ }
+
+ memcpy(out, data_, total_bytes);
+ data_ += total_bytes;
+ size_ -= total_bytes;
+
+ if constexpr (swap) {
+ for (uint32_t i = 0; i < n; i++) {
+ out[i] = ::arrow::bit_util::ByteSwap(out[i]);
+ }
+ }
+ }
+
+ size_t size() { return size_; }
+
+ private:
+ const uint8_t* data_;
+ size_t size_;
+};
+
+struct BoundingBox {
+ BoundingBox(Dimensions::dimensions dimensions, const std::array<double, 4>& mins,
+ const std::array<double, 4>& maxes)
+ : dimensions(dimensions) {
+ std::memcpy(min, mins.data(), sizeof(min));
+ std::memcpy(max, maxes.data(), sizeof(max));
+ }
+ explicit BoundingBox(Dimensions::dimensions dimensions = Dimensions::XYZM)
+ : dimensions(dimensions),
+ min{kInf, kInf, kInf, kInf},
+ max{-kInf, -kInf, -kInf, -kInf} {}
+
+ BoundingBox(const BoundingBox& other) = default;
+ BoundingBox& operator=(const BoundingBox&) = default;
+
+ void Reset() {
+ for (int i = 0; i < 4; i++) {
+ min[i] = kInf;
+ max[i] = -kInf;
+ }
+ }
+
+ void Merge(const BoundingBox& other) {
+ if (ARROW_PREDICT_TRUE(dimensions == other.dimensions)) {
+ for (int i = 0; i < 4; i++) {
+ min[i] = std::min(min[i], other.min[i]);
+ max[i] = std::max(max[i], other.max[i]);
+ }
+
+ return;
+ } else if (dimensions == Dimensions::XYZM) {
+ Merge(other.ToXYZM());
+ } else {
+ ParquetException::NYI();
+ }
+ }
+
+ BoundingBox ToXYZM() const {
+ BoundingBox xyzm(Dimensions::XYZM);
+ auto to_xyzm = Dimensions::ToXYZM(dimensions);
+ for (int i = 0; i < 4; i++) {
+ int dim_to_xyzm = to_xyzm[i];
+ if (dim_to_xyzm == -1) {
+ xyzm.min[i] = kInf;
+ xyzm.max[i] = -kInf;
+ } else {
+ xyzm.min[i] = min[dim_to_xyzm];
+ xyzm.max[i] = max[dim_to_xyzm];
+ }
+ }
+
+ return xyzm;
+ }
+
+ std::string ToString() const {
+ std::stringstream ss;
+ ss << "BoundingBox " << Dimensions::ToString(dimensions) << " [" << min[0] << " => "
+ << max[0];
+ for (int i = 1; i < 4; i++) {
+ ss << ", " << min[i] << " => " << max[i];
+ }
+
+ ss << "]";
+
+ return ss.str();
+ }
+
+ Dimensions::dimensions dimensions;
+ double min[4];
+ double max[4];
+};
+
+inline bool operator==(const BoundingBox& lhs, const BoundingBox& rhs) {
+ return lhs.dimensions == rhs.dimensions &&
+ std::memcmp(lhs.min, rhs.min, sizeof(lhs.min)) == 0 &&
+ std::memcmp(lhs.max, rhs.max, sizeof(lhs.max)) == 0;
+}
+
+template <Dimensions::dimensions dims, bool swap, uint32_t chunk_size>
+class WKBSequenceBounder {
+ public:
+ explicit WKBSequenceBounder(double* chunk) : box_(dims), chunk_(chunk) {}
+
+ void ReadPoint(WKBBuffer* src) {
+ constexpr uint32_t coord_size = Dimensions::size<dims>();
+ src->ReadDoubles<swap>(coord_size, chunk_);
+ for (uint32_t dim = 0; dim < coord_size; dim++) {
+ if (ARROW_PREDICT_TRUE(!std::isnan(chunk_[dim]))) {
+ box_.min[dim] = std::min(box_.min[dim], chunk_[dim]);
+ box_.max[dim] = std::max(box_.max[dim], chunk_[dim]);
+ }
+ }
+ }
+
+ void ReadSequence(WKBBuffer* src) {
+ constexpr uint32_t coord_size = Dimensions::size<dims>();
+ constexpr uint32_t coords_per_chunk = chunk_size / coord_size;
+
+ uint32_t n_coords = src->ReadUInt32<swap>();
+ uint32_t n_chunks = n_coords / coords_per_chunk;
+ for (uint32_t i = 0; i < n_chunks; i++) {
+ src->ReadDoubles<swap>(coords_per_chunk, chunk_);
+ ReadChunk(coords_per_chunk);
+ }
+
+ uint32_t remaining_coords = n_coords - (n_chunks * coords_per_chunk);
+ src->ReadDoubles<swap>(remaining_coords * coord_size, chunk_);
+ ReadChunk(remaining_coords);
+ }
+
+ void ReadRings(WKBBuffer* src) {
+ uint32_t n_rings = src->ReadUInt32<swap>();
+ for (uint32_t i = 0; i < n_rings; i++) {
+ ReadSequence(src);
+ }
+ }
+
+ void Reset() { box_.Reset(); }
+
+ void Finish(BoundingBox* out) { out->Merge(box_); }
+
+ private:
+ BoundingBox box_;
+ double* chunk_;
+
+ void ReadChunk(uint32_t n_coords) {
+ constexpr uint32_t coord_size = Dimensions::size<dims>();
+ for (uint32_t dim = 0; dim < coord_size; dim++) {
+ for (uint32_t i = 0; i < n_coords; i++) {
+ box_.min[dim] = std::min(box_.min[dim], chunk_[i * coord_size + dim]);
+ box_.max[dim] = std::max(box_.max[dim], chunk_[i * coord_size + dim]);
+ }
+ }
+ }
+};
+
+// We could (should?) avoid this madness by not templating the WKBSequenceBounder
+class WKBGenericSequenceBounder {
+ public:
+ WKBGenericSequenceBounder()
+ : xy_(chunk_),
Review Comment:
This is me being fancy and trying to avoid branching 🙂 . A runtime switch is probably fine for a POC and it's possibly worth benchmarking before introducing this complexity.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1746462982
##########
cpp/src/parquet/statistics.cc:
##########
@@ -47,6 +48,155 @@ using arrow::util::SafeCopy;
using arrow::util::SafeLoad;
namespace parquet {
+
+class GeometryStatisticsImpl {
+ public:
+ bool Equals(const GeometryStatisticsImpl& other) const {
+ if (is_valid_ != other.is_valid_) {
+ return false;
+ }
+
+ if (!is_valid_ && !other.is_valid_) {
+ return true;
+ }
+
+ auto wkb_types = bounder_.WkbTypes();
+ auto other_wkb_types = other.bounder_.WkbTypes();
+ if (wkb_types.size() != other_wkb_types.size()) {
+ return false;
+ }
+
+ for (size_t i = 0; i < wkb_types.size(); i++) {
+ if (wkb_types[i] != other_wkb_types[i]) {
+ return false;
+ }
+ }
+
+ return bounder_.Bounds() == other.bounder_.Bounds();
+ }
+
+ void Merge(const GeometryStatisticsImpl& other) {
+ if (!is_valid_ || !other.is_valid_) {
+ is_valid_ = false;
+ return;
+ }
+
+ bounder_.ReadBox(other.bounder_.Bounds());
+ bounder_.ReadGeometryTypes(other.bounder_.WkbTypes());
+ }
+
+ void Update(const ByteArray* values, int64_t num_values, int64_t null_count) {
Review Comment:
I've added UpdateSpaced in https://github.com/apache/arrow/pull/43977/commits/fb134d3af036ee5e240c7475477049c28bd6951c, will add tests for it later.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1722114973
##########
cpp/src/parquet/types.h:
##########
@@ -166,6 +167,14 @@ class PARQUET_EXPORT LogicalType {
enum unit { UNKNOWN = 0, MILLIS = 1, MICROS, NANOS };
};
+ struct GeometryEncoding {
Review Comment:
This follows the pattern of the other enums in types.h, which I am guessing would be enum classes if they were written today (maybe dating from a C++11 or pre-C++11 era?).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1745868930
##########
cpp/src/parquet/statistics.h:
##########
@@ -135,6 +186,7 @@ class PARQUET_EXPORT EncodedStatistics {
bool has_max = false;
bool has_null_count = false;
bool has_distinct_count = false;
+ bool has_geometry_statistics = false;
Review Comment:
My sense would be to handle hanging that pattern with a dedicated PR but I have no opinions either way 🙂
##########
cpp/src/parquet/statistics.h:
##########
@@ -135,6 +186,7 @@ class PARQUET_EXPORT EncodedStatistics {
bool has_max = false;
bool has_null_count = false;
bool has_distinct_count = false;
+ bool has_geometry_statistics = false;
Review Comment:
My sense would be to handle changing that pattern with a dedicated PR but I have no opinions either way 🙂
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1746401553
##########
cpp/src/parquet/statistics.h:
##########
@@ -372,11 +435,14 @@ template <typename DType>
std::shared_ptr<TypedStatistics<DType>> MakeStatistics(
const ColumnDescriptor* descr, const std::string& encoded_min,
const std::string& encoded_max, int64_t num_values, int64_t null_count,
- int64_t distinct_count, bool has_min_max, bool has_null_count,
- bool has_distinct_count, ::arrow::MemoryPool* pool = ::arrow::default_memory_pool()) {
- return std::static_pointer_cast<TypedStatistics<DType>>(Statistics::Make(
- descr, encoded_min, encoded_max, num_values, null_count, distinct_count,
- has_min_max, has_null_count, has_distinct_count, pool));
+ int64_t distinct_count, const EncodedGeometryStatistics& geometry_statistics,
Review Comment:
Do we need to make it ABI-compatible or source-compatible? If ABI compatibility is required we must add a new override version of this function.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1722126641
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
+ if (type.GEOMETRY.edges == format::Edges::PLANAR) {
+ edges = LogicalType::GeometryEdges::PLANAR;
+ } else if (type.GEOMETRY.edges == format::Edges::SPHERICAL) {
+ edges = LogicalType::GeometryEdges::SPHERICAL;
+ }
+
+ LogicalType::GeometryEncoding::geometry_encoding encoding =
+ LogicalType::GeometryEncoding::UNKNOWN;
+ if (type.GEOMETRY.encoding == format::GeometryEncoding::WKB) {
+ encoding = LogicalType::GeometryEncoding::WKB;
+ }
+
+ std::string metadata;
Review Comment:
I believe that both string and binary come through C++ thrift as `std::string`. Happy to have this be whatever but I think `std::string` is consistent with our plans for its contents (JSON?).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1746462769
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
+ if (type.GEOMETRY.edges == format::Edges::PLANAR) {
+ edges = LogicalType::GeometryEdges::PLANAR;
+ } else if (type.GEOMETRY.edges == format::Edges::SPHERICAL) {
+ edges = LogicalType::GeometryEdges::SPHERICAL;
+ }
+
+ LogicalType::GeometryEncoding::geometry_encoding encoding =
+ LogicalType::GeometryEncoding::UNKNOWN;
Review Comment:
addressed in https://github.com/apache/arrow/pull/43977/commits/fb134d3af036ee5e240c7475477049c28bd6951c
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1746462645
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
Review Comment:
addressed in https://github.com/apache/arrow/pull/43977/commits/fb134d3af036ee5e240c7475477049c28bd6951c
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1746629470
##########
cpp/src/parquet/statistics.h:
##########
@@ -372,11 +435,14 @@ template <typename DType>
std::shared_ptr<TypedStatistics<DType>> MakeStatistics(
const ColumnDescriptor* descr, const std::string& encoded_min,
const std::string& encoded_max, int64_t num_values, int64_t null_count,
- int64_t distinct_count, bool has_min_max, bool has_null_count,
- bool has_distinct_count, ::arrow::MemoryPool* pool = ::arrow::default_memory_pool()) {
- return std::static_pointer_cast<TypedStatistics<DType>>(Statistics::Make(
- descr, encoded_min, encoded_max, num_values, null_count, distinct_count,
- has_min_max, has_null_count, has_distinct_count, pool));
+ int64_t distinct_count, const EncodedGeometryStatistics& geometry_statistics,
Review Comment:
I think API-compatible is sufficient. Please correct me if I'm wrong @pitrou
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #43196:
URL: https://github.com/apache/arrow/pull/43196#issuecomment-2323417770
Sorry for the slow reply, I've been out of office! You are of course welcome to continue on top of this PR...I'm back in office Thursday and can definitely review. I think the main remaining portion is the Arrow IO and serde of the statistics.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #43196:
URL: https://github.com/apache/arrow/pull/43196#issuecomment-2327043515
@paleolimbot Thanks for the message. We will soon create a PR against your branch!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1720790638
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
+ if (type.GEOMETRY.edges == format::Edges::PLANAR) {
+ edges = LogicalType::GeometryEdges::PLANAR;
+ } else if (type.GEOMETRY.edges == format::Edges::SPHERICAL) {
+ edges = LogicalType::GeometryEdges::SPHERICAL;
+ }
+
+ LogicalType::GeometryEncoding::geometry_encoding encoding =
+ LogicalType::GeometryEncoding::UNKNOWN;
+ if (type.GEOMETRY.encoding == format::GeometryEncoding::WKB) {
+ encoding = LogicalType::GeometryEncoding::WKB;
+ }
+
+ std::string metadata;
+ if (type.GEOMETRY.__isset.crs) {
Review Comment:
```suggestion
if (type.GEOMETRY.__isset.metadata) {
```
##########
cpp/src/parquet/statistics.h:
##########
@@ -24,6 +24,7 @@
#include <string>
#include <utility>
+#include "parquet/geometry_util.h"
Review Comment:
Could we use forward declaration? This is a public header.
##########
cpp/src/parquet/types.h:
##########
@@ -166,6 +167,14 @@ class PARQUET_EXPORT LogicalType {
enum unit { UNKNOWN = 0, MILLIS = 1, MICROS, NANOS };
};
+ struct GeometryEncoding {
Review Comment:
Should we use `enum class GeometryEncoding` instead?
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
+ if (type.GEOMETRY.edges == format::Edges::PLANAR) {
+ edges = LogicalType::GeometryEdges::PLANAR;
+ } else if (type.GEOMETRY.edges == format::Edges::SPHERICAL) {
+ edges = LogicalType::GeometryEdges::SPHERICAL;
+ }
+
+ LogicalType::GeometryEncoding::geometry_encoding encoding =
+ LogicalType::GeometryEncoding::UNKNOWN;
+ if (type.GEOMETRY.encoding == format::GeometryEncoding::WKB) {
+ encoding = LogicalType::GeometryEncoding::WKB;
+ }
+
+ std::string metadata;
Review Comment:
The `metadata` field has been changed from string to binary type as suggested by @emkornfield to be more flexible (e.g. encoding):
https://github.com/apache/parquet-format/pull/240#discussion_r1639285008. I'm not sure if this makes implementation difficult to deal with the complexity.
##########
cpp/src/parquet/geometry_util.h:
##########
@@ -0,0 +1,603 @@
+// 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.
+
+#pragma once
+
+#include <algorithm>
+#include <limits>
+#include <string>
+#include <unordered_set>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/ubsan.h"
+#include "parquet/exception.h"
+
+namespace parquet::geometry {
+
+constexpr double kInf = std::numeric_limits<double>::infinity();
+
+struct Dimensions {
+ enum dimensions { XY = 0, XYZ = 1, XYM = 2, XYZM = 3 };
+
+ static dimensions FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type / 1000) {
+ case 0:
+ return XY;
+ case 1:
+ return XYZ;
+ case 2:
+ return XYM;
+ case 3:
+ return XYZM;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ template <dimensions dims>
+ constexpr static uint32_t size();
+
+ template <>
+ constexpr uint32_t size<XY>() {
+ return 2;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZ>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYM>() {
+ return 3;
+ }
+
+ template <>
+ constexpr uint32_t size<XYZM>() {
+ return 4;
+ }
+
+ static uint32_t size(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return size<XY>();
+ case XYZ:
+ return size<XYZ>();
+ case XYM:
+ return size<XYM>();
+ case XYZM:
+ return size<XYZM>();
+ default:
+ return 0;
+ }
+ }
+
+ // Where to look in a coordinate with this dimension
+ // for the X, Y, Z, and M dimensions, respectively.
+ static std::array<int, 4> ToXYZM(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return {0, 1, -1, -1};
+ case XYZ:
+ return {0, 1, 2, -1};
+ case XYM:
+ return {0, 1, -1, 2};
+ case XYZM:
+ return {0, 1, 2, 3};
+ default:
+ return {-1, -1, -1, -1};
+ }
+ }
+
+ static std::string ToString(dimensions dims) {
+ switch (dims) {
+ case XY:
+ return "XY";
+ case XYZ:
+ return "XYZ";
+ case XYM:
+ return "XYM";
+ case XYZM:
+ return "XYZM";
+ default:
+ return "";
+ }
+ }
+};
+
+struct GeometryType {
+ enum geometry_type {
+ POINT = 1,
+ LINESTRING = 2,
+ POLYGON = 3,
+ MULTIPOINT = 4,
+ MULTILINESTRING = 5,
+ MULTIPOLYGON = 6,
+ GEOMETRYCOLLECTION = 7
+ };
+
+ static geometry_type FromWKB(uint32_t wkb_geometry_type) {
+ switch (wkb_geometry_type % 1000) {
+ case 1:
+ return POINT;
+ case 2:
+ return LINESTRING;
+ case 3:
+ return POLYGON;
+ case 4:
+ return MULTIPOINT;
+ case 5:
+ return MULTILINESTRING;
+ case 6:
+ return MULTIPOLYGON;
+ case 7:
+ return GEOMETRYCOLLECTION;
+ default:
+ throw ParquetException("Invalid wkb_geometry_type: ", wkb_geometry_type);
+ }
+ }
+
+ static std::string ToString(geometry_type geometry_type) {
+ switch (geometry_type) {
+ case POINT:
+ return "POINT";
+ case LINESTRING:
+ return "LINESTRING";
+ case POLYGON:
+ return "POLYGON";
+ case MULTIPOINT:
+ return "MULTIPOINT";
+ case MULTILINESTRING:
+ return "MULTILINESTRING";
+ case MULTIPOLYGON:
+ return "MULTIPOLYGON";
+ case GEOMETRYCOLLECTION:
+ return "GEOMETRYCOLLECTION";
+ default:
+ return "";
+ }
+ }
+};
+
+class WKBBuffer {
Review Comment:
Should we move them into arrow/util instead of parquet only?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WIP: Proof-of-concept Parquet GEOMETRY logical type implementation [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #43196:
URL: https://github.com/apache/arrow/pull/43196#discussion_r1745870313
##########
cpp/src/parquet/types.cc:
##########
@@ -463,6 +463,31 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
return UUIDLogicalType::Make();
} else if (type.__isset.FLOAT16) {
return Float16LogicalType::Make();
+ } else if (type.__isset.GEOMETRY) {
+ std::string crs;
+ if (type.GEOMETRY.__isset.crs) {
+ crs = type.GEOMETRY.crs;
+ }
+
+ LogicalType::GeometryEdges::edges edges = LogicalType::GeometryEdges::UNKNOWN;
Review Comment:
I think I was emulating how enum parameters are handled in the date/time types here (but I may not have been!)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org