-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-38558: [C++] Add support for null sort option per sort key #46926
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
Open
Taepper
wants to merge
56
commits into
apache:main
Choose a base branch
from
Taepper:GH-38558
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,792
−648
Open
Changes from all commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
a62b1f7
Fix: Add support for null sort option per sort key.
Light-City d8bca7c
fix test
Light-City 970f5bf
fix sortkey assert
Light-City 6c0bc76
fix serde test
Light-City 25984c5
better api
Taepper c07f37a
Merge remote-tracking branch 'refs/remotes/fork/main' into GH-38558
Taepper 241fbee
Merge branch 'refs/heads/better-api' into GH-38558
Taepper dafbe14
merge follow-up
Taepper 48a3e0c
merge follow-up
Taepper 5c9eb50
formatting
Taepper 12d1b0d
formatting
Taepper 0b442f7
missing unwrap_null_placement in python
Taepper 4398d16
do not remove demoted null_placement from python api
Taepper bdc4069
fix member name in hash_aggregate_test
Taepper 8fc630c
update ToString method output
Taepper dcb3b7a
format remove extra empty line
Taepper 682a37a
fix python interface
Taepper e6afb8a
python formatting
Taepper 82299bc
do not pass None to python C binding
Taepper 370cdac
format python
Taepper 503a7d1
fix minor python api mistakes
Taepper 1d0883e
python formatting
Taepper 7833add
do not rename member of exposed API struct
Taepper 6e2759b
do not rename member of exposed API struct, missed test file
Taepper 0a029ca
format cc file
Taepper 3463a59
updating api that was not using additional argument for some reason (…
Taepper e01a009
make null_placement optional in the python acero api
Taepper 794a2cd
minor additional fixes
Taepper adaae37
amend c_glib api to use std::optional<NullPlacement> for RankOptions
Taepper 14c3914
amend c_glib api to use std::optional<NullPlacement> for RankOptions
Taepper 791d8e9
amend c_glib api to use std::optional<NullPlacement> for RankOptions
Taepper bee7e81
amend c_glib api to use std::optional<NullPlacement> for RankOptions
Taepper cbfd596
fix mistake where default null placement was AtStart
Taepper 779169e
fix mistake where null_placement was not correctly set when sort_keys…
Taepper dbd75d0
fix c_glib bindings
Taepper 67e99ac
formatting
Taepper e26fbc4
formatting
Taepper 1dd5079
Merge remote-tracking branch 'origin/main' into GH-38558
Taepper 72a28f9
Merge branch 'main' into GH-38558
Taepper e07a673
also update RankQuantileOptions to new NullPlacement api
Taepper 290812f
update library version documentation
Taepper 5320622
rename GARROW_OPTIONAL_NULL_PLACEMENT_UNSET to GARROW_OPTIONAL_NULL_P…
Taepper bf31270
formatting
Taepper 2e58463
ruby default value formatting
Taepper 79995df
change red-arrow gem to use different syntax for specifying null_plac…
Taepper bac578a
update ruby test functions that use direct GObject introspection
Taepper 1411115
simplify helper functions
Taepper 8f041b5
move and export helper functions
Taepper d36fbb6
fixup move and export helper functions
Taepper 272ec3f
format ruby
Taepper f1fda48
fix another ruby test
Taepper e398fe4
fix another ruby test
Taepper 4778d97
Improve `GArrowOptionalNullPlacement` documentation
Taepper 15542b0
Make static_cast of GARROW_OPTIONAL_NULL_PLACEMENT safer by having GA…
Taepper b7293cb
improve placement of helper garrow_optional_null_placement_* helper f…
Taepper b33465e
more ruby test fixes
Taepper File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
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.
This would be missing a static_cast:
Are you sure this is the cleaner way to go? Should I replace all usage of the helper function
garrow_optional_null_placement_to_rawlike this?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.
Ah, we have multiple places that use these helper functions. I missed them. Sorry.
Could you export these helper functions from
compute.hppinstead of defining them in an anonymous namespace? And could you simplify these helper functions?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.
Will do
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.
Okay, I did this, but this lead to the following change:
The return type is a cpp object and not a pointer as for most other exported functions. Therefore, its definition must be outside the
G_BEGIN_DECLS/G_END_DECLS(these defineextern "C"). I moved it there, but it makes it feel a little out of place. No other definition before the lineG_BEGIN_DECLSis exported.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.
Could you move them after the
G_END_DECLSarrow/c_glib/arrow-glib/compute.cpp
Line 10666 in 75ef031
G_END_DECLS.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.
Oh, this was embarrassing to miss. Thanks for the pointer!