Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[C++] Refactor dict_internal.h to use Result #36111

Closed
bkietz opened this issue Jun 15, 2023 · 4 comments · Fixed by #37754
Closed

[C++] Refactor dict_internal.h to use Result #36111

bkietz opened this issue Jun 15, 2023 · 4 comments · Fixed by #37754
Assignees
@chrisjordansquire
Labels
Component: C++ good-first-issue Type: enhancement
Milestone
14.0.0

Comments

@bkietz
Copy link
Member

bkietz commented Jun 15, 2023

Describe the enhancement requested

DictionaryTraits<>::GetDictionaryArrayData still uses the Status(out ptr) pattern instead of returning Result<Out>, which is less readable.

Component(s)

C++
@bkietz bkietz mentioned this issue Jun 15, 2023
Closed
@infdahai
Copy link
Contributor

infdahai commented Jun 20, 2023 •
edited
Loading

I'd like to take this task.

@bkietz
Copy link
Member Author

bkietz commented Jun 20, 2023

If you write a PR, @mention me and I'll review

@infdahai
Copy link
Contributor

I get it

@github-actions github-actions bot mentioned this issue Jul 2, 2023
Merged
chrisjordansquire added a commit to chrisjordansquire/arrow that referenced this issue Sep 16, 2023
@chrisjordansquire
dict_internal.h uses Result Instead of Status
b0141bd 
Resolve apache#36111 by refactoring dict_internal.h to return
a Result instead of a Status type. This narrow refactor
minimally impacts the usage sites.
@chrisjordansquire chrisjordansquire mentioned this issue Sep 16, 2023
Merged
@chrisjordansquire
Copy link
Contributor

@bkietz - I made a PR for this issue at #37754 . I'm unsure if I correctly interpreted the task. Feedback greatly appreciated.

bkietz pushed a commit that referenced this issue Sep 20, 2023
@chrisjordansquire
GH-36111: [C++] Refactor dict_internal.h to use Result (#37754)
868b9bd 
### Rationale for this change

This change addresses #36111 , updating the method GetDictionaryArrayData in dict_internal.h to return a Result instead of a Status type. 

### What changes are included in this PR?

This is a narrow interpretation of the above issue, only changing the return type with minimal updates to the call sites or their return types. 

### Are these changes tested?

Yes. The C++ test suite was run on the `ninja-debug` build target using the command `ctest -j16 --output-on-failure`. All tests not requiring parquet data passed. (I was unsure how to setup those tests based on the Arrow development guidelines.) 

### Are there any user-facing changes?

No. The only changes should be library-internal. 

* Closes: #36111 

Authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
@bkietz bkietz added this to the 14.0.0 milestone Sep 20, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
@chrisjordansquire @loicalleyne
apacheGH-36111: [C++] Refactor dict_internal.h to use Result (apache#…
3cbe4f7 
…37754)

### Rationale for this change

This change addresses apache#36111 , updating the method GetDictionaryArrayData in dict_internal.h to return a Result instead of a Status type. 

### What changes are included in this PR?

This is a narrow interpretation of the above issue, only changing the return type with minimal updates to the call sites or their return types. 

### Are these changes tested?

Yes. The C++ test suite was run on the `ninja-debug` build target using the command `ctest -j16 --output-on-failure`. All tests not requiring parquet data passed. (I was unsure how to setup those tests based on the Arrow development guidelines.) 

### Are there any user-facing changes?

No. The only changes should be library-internal. 

* Closes: apache#36111 

Authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@chrisjordansquire @dgreiss
apacheGH-36111: [C++] Refactor dict_internal.h to use Result (apache#…
d449943 
…37754)

### Rationale for this change

This change addresses apache#36111 , updating the method GetDictionaryArrayData in dict_internal.h to return a Result instead of a Status type. 

### What changes are included in this PR?

This is a narrow interpretation of the above issue, only changing the return type with minimal updates to the call sites or their return types. 

### Are these changes tested?

Yes. The C++ test suite was run on the `ninja-debug` build target using the command `ctest -j16 --output-on-failure`. All tests not requiring parquet data passed. (I was unsure how to setup those tests based on the Arrow development guidelines.) 

### Are there any user-facing changes?

No. The only changes should be library-internal. 

* Closes: apache#36111 

Authored-by: Chris Jordan-Squire <chris.jordansquire@gmail.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chrisjordansquire chrisjordansquire

Labels
Component: C++ good-first-issue Type: enhancement
Projects
None yet
Milestone
14.0.0
Development

Successfully merging a pull request may close this issue.

GH-36111: [C++] Refactor dict_internal.h to use Result chrisjordansquire/arrow
3 participants
@chrisjordansquire @bkietz @infdahai