Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GH-32723: [C++][Parquet] Add option to use LARGE* variants of binary types #35825
base: main
Are you sure you want to change the base?
GH-32723: [C++][Parquet] Add option to use LARGE* variants of binary types #35825
Changes from all commits
e5e96ec
b9b48f8
ae62954
34917d5
835b07d
50427c6
df65ce7
e826b8e
764ef98
c6244ea
5a4bbb0
2d84e57
90f14df
b88b024
0b53b05
f574e2e
295e062
25d7815
fe8d67b
35e5835
9aff2f3
eb850c4
c2aab63
837ed6c
35cdb99
a5000e1
eb71c17
686a3f7
a61fc32
e2600d0
3b86e23
cc027b7
177db7a
66223ee
5cd39d8
1089010
a6c42ee
15be2a2
8d5ba3d
fd8f979
a5736d5
b4ecd0d
9e9dff9
ae1db20
09a9eaf
1664983
322319e
1775a7a
7f6e2bf
75fb615
0801267
7f09a16
a8d20a4
5fcf4e1
8901cbc
dff017a
232e01f
d7d76c6
90ceb07
0394963
a8df2e7
2bb3b14
c114d44
d1d5798
0eaa60f
1e642fa
b299497
2c23dd7
eca9d6f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't understand this condition. Which platforms is it excluding and why?
Large binary data is supposed to work on every platform, so there should be no reason to skip some platforms 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.
Again, it does not seem right that you are restricting tests that used to work on every platform (and that have no obvious reason to fail on some platforms).
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 still don't understand what
__LP64__
is doing here. If you really want to single out 64-bit platforms, you could instead do something like: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 also see that this test takes 18 seconds in debug mode. This seems a bit excessive :-/
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.
Looks like you could factor this out in the test fixture.
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 keep the XXX because it is a gotcha.
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.
Good to know something new :)