ORC-1920: [C++] Support `Geometry` and `Geography` types by ffacs · Pull Request #2269 · apache/orc

ffacs · 2025-06-16T10:02:20Z

What changes were proposed in this pull request?
Support Geometry and Geography types for c++ side

Why are the changes needed?
Add support for Geometry and Geography types

How was this patch tested?
UT passed

Was this patch authored or co-authored using generative AI tooling?
No

dongjoon-hyun

Thank you, @ffacs .

cc @wgtmac and @williamhyun

dongjoon-hyun · 2025-06-17T15:01:19Z

+ ///
+ /// These values correspond to the 1, 2, ..., 7 component of the WKB integer
+ /// geometry type (i.e., the value of geometry_type % 1000).
+ enum class GeometryType {


In Java, we had an additional value, -1, for UNKNOWN_TYPE_ID.
orc/java/core/src/java/org/apache/orc/geospatial/GeospatialTypes.java
Line 42 in 1e5c161
privatestaticfinalintUNKNOWN_TYPE_ID = -1;

C++ employs a different implementation that invalidates statistics when unknown types are encountered; therefore, UNKNOWN_TYPE_ID is unnecessary in this context.

dongjoon-hyun · 2025-06-17T15:12:37Z

+ }
+
+ std::stringstream ss;
+ ss << "<GeoStatistics>";


ditto. Why do we have this only in C++ implementation? If this is required, can we have this in Java first?

I think it is due to gap between Parquet Java and C++ implementations...

Yes, it is due to gap between Parquet Java and C++ implementations..

dongjoon-hyun

I have a few minor comments about the feature parity between Java and C++ implementations.

williamhyun

+1, thank you @ffacs for this PR, waiting on the above comment before merging.

dongjoon-hyun · 2025-06-23T18:45:22Z

Gentle ping, @ffacs .

ffacs · 2025-06-24T03:32:21Z

Gentle ping, @ffacs .

Pong. Thank you for your review @dongjoon-hyun , I'd update this patch these days.

dongjoon-hyun · 2025-06-24T14:08:35Z

Thank you, @ffacs . Also, please participate the 1.8.10 vote too when you have some time.

[VOTE] Release Apache ORC 1.8.10 (RC0)

wgtmac · 2025-06-24T14:53:54Z

+ }
+
+ std::stringstream ss;
+ ss << "<GeoStatistics>";


I think it is due to gap between Parquet Java and C++ implementations...

dongjoon-hyun · 2025-06-30T15:32:27Z

Could you fix the above comments, @ffacs ?

wgtmac · 2025-07-03T04:14:37Z

Let me know when ready to review.

ffacs · 2025-07-03T06:57:59Z

@wgtmac @dongjoon-hyun I'm ready to review now, please take a look when you're free~

dongjoon-hyun · 2025-07-04T00:59:11Z

Thank you, @ffacs .

dongjoon-hyun · 2025-07-04T01:02:26Z

+ * This file contains code adapted from the Apache Arrow project.
+ *
+ * Original source:
+ * https://github.com/apache/arrow/blob/main/cpp/src/parquet/geospatial/statistics.h


Please provide a tag based location instead of the main branch because it changes always.

There are no tags that contain this patch yet.

dongjoon-hyun · 2025-07-04T01:02:40Z

+ * This file contains code adapted from the Apache Arrow project.
+ *
+ * Original source:
+ * https://github.com/apache/arrow/blob/main/cpp/src/parquet/geospatial/statistics.cc


Please provide a tag based location instead of the main branch because it changes always.

dongjoon-hyun

+1, LGTM.

wgtmac · 2025-07-04T02:04:30Z

+ virtual const std::string& getCRS() const = 0;
+ virtual geospatial::EIAlgo getEIAlgo() const = 0;


Suggested change
virtualconst std::string& getCRS() const = 0;
virtual geospatial::EIAlgo getEIAlgo() const = 0;
virtualconst std::string& getCrs() const = 0;
virtual geospatial::EdgeInterpolationAlgorithm getAlgorithm() const = 0;
It would be good to add comment to say these two functions are for geometry & geography types only.

wgtmac · 2025-07-04T02:26:48Z

+ void WKBGeometryBounder::mergeGeometryInternal(WKBBuffer* src, bool recordWkbType) {
+ uint8_t endian = src->ReadUInt8();
+ bool swap = endian != 0x00;
+ if (isLittleEndian()) {


Perhaps we can use if constexpr (std::endian::native == std::endian::little)

std::endian requires C++20, we are using c++17 now.

dongjoon-hyun · 2025-07-11T21:18:09Z

Is this ready or do we need more revision, @ffacs and @wgtmac ?

wgtmac · 2025-07-13T02:39:42Z

Is this ready or do we need more revision, @ffacs and @wgtmac ?

My only concern is the name of public api: https://github.com/apache/orc/pull/2269/files#r2184135226. It is better to use getCrs() and getAlgorithm()/getEdgeAlgorithm()

dongjoon-hyun · 2025-07-14T00:31:14Z

Thank you everyone, @ffacs , @williamhyun , @wgtmac .
Merged to main/2.2.

What changes were proposed in this pull request? Support Geometry and Geography types for c++ side Why are the changes needed? Add support for Geometry and Geography types How was this patch tested? UT passed Was this patch authored or co-authored using generative AI tooling? No Closes#2269 from ffacs/geo. Authored-by: ffacs <ffacs520@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit ca9bb59) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

### What changes were proposed in this pull request? This PR aims to upgrade ORC to 2.2.0. 2.2.0 RC1 is currently under voting. ### Why are the changes needed? Apache ORC 2.2.0 is a new feature release. - https://github.com/apache/orc/releases/tag/v2.2.0 - apache/orc#2032 - apache/orc#2338 - apache/orc#2269 - apache/orc#2249 - apache/orc#2144 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes#51676 from williamhyun/ORC-2.2.0. Authored-by: William Hyun <william@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

ffacs changed the title ~~ORC-1920: Support Geometry and Geography types~~ORC-1920: [C++] Support Geometry and Geography typesJun 16, 2025

github-actionsBot added the CPP label Jun 16, 2025

ffacs changed the title ~~ORC-1920: [C++] Support Geometry and Geography types~~ORC-1920: [C++] Support Geometry and Geography typesJun 16, 2025

dongjoon-hyun reviewed Jun 16, 2025
View reviewed changes

dongjoon-hyun added this to the 2.2.0 milestone Jun 16, 2025

dongjoon-hyun reviewed Jun 17, 2025
View reviewed changes

Comment threadc++/include/orc/Geospatial.hh

dongjoon-hyun reviewed Jun 17, 2025
View reviewed changes

Comment threadc++/src/Geospatial.cc Outdated

dongjoon-hyun reviewed Jun 17, 2025
View reviewed changes

williamhyun approved these changes Jun 19, 2025
View reviewed changes

wgtmac reviewed Jun 24, 2025
View reviewed changes

ffacs added 8 commits July 2, 2025 00:28

[C++] Support and types
891e1dc

fix building
e507633

cstdint
95d6f5e

fix SEH
4bf2ac7

fix linter
4f0e742

fix linter for Geospatial.cc
176ea6b

fix linter for TypeImpl.hh
728f07e

Refactor coding style and address comments
e0421f9

ffacs force-pushed the geo branch from d3eb8b8 to e0421f9Compare July 1, 2025 16:29

ffacs added 2 commits July 2, 2025 00:34

Fix meson build
4b15527

fix meson build again
4c7c05d

ffacs added 2 commits July 3, 2025 13:55

fix linter
7b32319

Split Geospatial.hh
f4d7c27

ffacs added 2 commits July 3, 2025 14:16

Fix build
e8eb551

include cstdint
630020b

dongjoon-hyun reviewed Jul 4, 2025
View reviewed changes

Comment threadc++/include/orc/Statistics.hh

dongjoon-hyun reviewed Jul 4, 2025
View reviewed changes

Comment threadc++/include/orc/Statistics.hh Outdated

dongjoon-hyun reviewed Jul 4, 2025
View reviewed changes

dongjoon-hyun approved these changes Jul 4, 2025
View reviewed changes

wgtmac reviewed Jul 4, 2025
View reviewed changes

dongjoon-hyun mentioned this pull request Jul 5, 2025
Add support for geospatial types (geometry and geography) #2224
Closed

Address comments
e14391d

wgtmac reviewed Jul 13, 2025
View reviewed changes

Comment threadc++/src/TypeImpl.cc Outdated

rename public api
39427e9

wgtmac approved these changes Jul 13, 2025
View reviewed changes

dongjoon-hyun closed this in ca9bb59Jul 14, 2025

dongjoon-hyun mentioned this pull request Jul 27, 2025
[SPARK-52967][BUILD] Upgrade ORC to 2.2.0 apache/spark#51676
Closed

		virtual const std::string& getCRS() const = 0;
		virtual geospatial::EIAlgo getEIAlgo() const = 0;

-virtualconst std::string& getCRS() const = 0;
-virtual geospatial::EIAlgo getEIAlgo() const = 0;
+virtualconst std::string& getCrs() const = 0;
+virtual geospatial::EdgeInterpolationAlgorithm getAlgorithm() const = 0;

Uh oh!

Conversation

ffacs commented Jun 16, 2025

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

williamhyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jun 23, 2025

Uh oh!

ffacs commented Jun 24, 2025

Uh oh!

dongjoon-hyun commented Jun 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jun 30, 2025

Uh oh!

wgtmac commented Jul 3, 2025

Uh oh!

ffacs commented Jul 3, 2025

Uh oh!

dongjoon-hyun commented Jul 4, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 11, 2025

Uh oh!

Uh oh!

wgtmac commented Jul 13, 2025

Uh oh!

dongjoon-hyun commented Jul 14, 2025

Uh oh!

Reviewers

Assignees