-
Notifications
You must be signed in to change notification settings - Fork 4.1k
GH-45522: [Parquet][C++] Parquet GEOMETRY and GEOGRAPHY logical type implementations #45459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 100 commits
c3531f7
086e52c
c5d01e1
e9d5180
8487f71
983d6b6
2a80461
2c4f7e3
03dbac4
c460eb0
2d6d5cb
568e76c
0630edd
a158be4
3e5c097
78c4cb2
ab1a0c0
014eb06
c62393f
a6ebbee
d8bb4f6
06c13f5
26329ac
1990275
ceffbc9
edcf971
f728191
790b6af
0b686a7
adc2b3d
4008b8f
f0e019e
fcc7af6
f4aedf6
6224fd4
b76541b
9040c3b
2780df0
ae4e1b8
e2cbc4f
32346b9
80b2328
a060b51
0014cf9
774fe19
d15bd2a
7657bd3
dfe15e7
a391819
8415d3b
21e1484
0d06228
13d7435
a28b611
66cadb8
942a591
d350a39
801aad0
b263b16
124a0f5
6da241b
4d5e539
bb5f9bb
7b97725
90609c2
e5163b0
8c591c9
1513336
3f17099
0578ae9
7b4ec55
a14fb07
246d071
cd0b1cc
80f9e81
b76563d
28a1dd1
0da26d1
a7b3c4b
cf4481f
c9c8b9d
85959a4
699eddd
a2ab3b6
c917d33
6ceca04
d11413b
a586ca8
11df2d1
e01ec4e
9f6baf2
ee13896
b6b96da
c386fd2
7aed73f
4c18e94
d438e3d
465308b
a86feee
bdbdcd1
0295268
749fd63
0af1de8
e0adea3
e7da4fd
32d6e31
df8adf7
96682c8
7614ff9
b0381b7
fd7f722
bac47dc
91b0872
9358f90
c1c5dc3
8c48088
c6ae22b
bc57c46
4611d4b
bea82e4
5b8d654
5af1471
b3261b2
e529480
90202bd
9b51ca3
0cbe770
7a7ceb1
995df92
c38a476
2b87131
e2f2044
516a053
73a47dc
92d655d
43dd64c
81567b9
94f1908
759ab2e
4ce6803
4bca1c0
e1b7061
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| #include "parquet/column_scanner.h" | ||
| #include "parquet/exception.h" | ||
| #include "parquet/file_reader.h" | ||
| #include "parquet/geospatial/statistics.h" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we want to further expand the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
| #include "parquet/metadata.h" | ||
| #include "parquet/platform.h" | ||
| #include "parquet/printer.h" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to modify the CMake config to enable
ARROW_JSONwhenARROW_PARQUETis ON? cc @kou @raulcdThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need JSON if
PARQUET_MINIMAL_DEPENDENCYisON, right?Then, we don't need the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why did we introduce
PARQUET_MINIMAL_DEPENDENCYfor? What is the purpose? It doesn't seem used or tested anywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that apache/parquet-cpp#279 introduced it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha... so we've had this option for ~8 years and it was never tested or documented anywhere. I've opened #46219 so that we may eventually remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARROW_JSONwill build the Arrow JSONL reader, which should not be required for Parquet. We just need RapidJSON here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest this diff:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: We can remove this
if(NOT ARROW_JSON)with the suggested change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!