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++] DataType::ToString should optionally show metadata #39864

Closed
pitrou opened this issue Jan 31, 2024 · 3 comments · Fixed by #39888
Closed

[C++] DataType::ToString should optionally show metadata #39864

pitrou opened this issue Jan 31, 2024 · 3 comments · Fixed by #39888
Assignees
@Cerdore
Labels
Component: C++ good-first-issue Type: enhancement
Milestone
16.0.0

Comments

@pitrou
Copy link
Member

pitrou commented Jan 31, 2024

Describe the enhancement requested

A DataType doesn't directly carry metadata, but if it is a nested DataType, some of its child fields might carry metadata.

Therefore, similar to Schema::ToString, DataType::ToString should take an optional show_metadata argument.

Component(s)

C++
@Cerdore
Copy link
Contributor

Cerdore commented Feb 1, 2024 •
edited
Loading

Hi! I'm interested in this issue and want to try implementing it!

@Cerdore
Copy link
Contributor

Cerdore commented Feb 1, 2024 •
edited
Loading

I have 2 questions to confirm.

  1. We should add an optional argument "show_metadata" to the ToString method of DataType and other DataType derived class. And we also need to add show_metadata to TypeHolder::ToString() , right?

  2. Default value of show_metadata should always be false in derived class. Because virtual functions are dynamically bound, but default parameter values are statically bound.

@pitrou
Copy link
Member Author

pitrou commented Feb 1, 2024 •
edited
Loading

  1. We should add an optional argument "show_metadata" to the ToString method of DataType and other DataType derived class. And we also need to add show_metadata to TypeHolder::ToString() , right?

Yes!

  1. Default value of show_metadata should always be false in derived class. Because virtual functions are dynamically bound, but default parameter values are statically bound.

I suppose so, yes. In any case, we want the API to be consistent, this makes life easier for users.

@Cerdore Cerdore mentioned this issue Feb 1, 2024
Merged
pitrou added a commit that referenced this issue Feb 27, 2024
@Cerdore @pitrou
GH-39864: [C++] DataType::ToString support optionally show metadata (#…
c5f60a0 
…39888)

### Rationale for this change

Support showing metadata of  nested DataType which have child fields.

### What changes are included in this PR?

Add an optional argument "show_metadata" to the ToString() of DataType and other DataType derived class. And we also  add it to TypeHolder::ToString().

### Are these changes tested?

Yes, I add tests for changes.

### Are there any user-facing changes?
No.

Closes: #39864
* Closes: #39864

Lead-authored-by: xiansen.chen <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@pitrou pitrou added this to the 16.0.0 milestone Feb 27, 2024
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
@Cerdore @pitrou @zanmato1984
apacheGH-39864: [C++] DataType::ToString support optionally show meta…
fde5998 
…data (apache#39888)

### Rationale for this change

Support showing metadata of  nested DataType which have child fields.

### What changes are included in this PR?

Add an optional argument "show_metadata" to the ToString() of DataType and other DataType derived class. And we also  add it to TypeHolder::ToString().

### Are these changes tested?

Yes, I add tests for changes.

### Are there any user-facing changes?
No.

Closes: apache#39864
* Closes: apache#39864

Lead-authored-by: xiansen.chen <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
@Cerdore @pitrou @thisisnic
apacheGH-39864: [C++] DataType::ToString support optionally show meta…
0b2cfae 
…data (apache#39888)

### Rationale for this change

Support showing metadata of  nested DataType which have child fields.

### What changes are included in this PR?

Add an optional argument "show_metadata" to the ToString() of DataType and other DataType derived class. And we also  add it to TypeHolder::ToString().

### Are these changes tested?

Yes, I add tests for changes.

### Are there any user-facing changes?
No.

Closes: apache#39864
* Closes: apache#39864

Lead-authored-by: xiansen.chen <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@assignUser assignUser mentioned this issue Apr 27, 2024
Merged
assignUser added a commit that referenced this issue Apr 29, 2024
@assignUser
GH-41398: [R][CI] Windows job failing after R 4.4 release (#41409)
00df70c 
### Rationale for this change

We can't throw warnings on cran.

### What changes are included in this PR?

Update function to match changes in libarrow added in GH-39864

### Are these changes tested?

CI
### Are there any user-facing changes?

No

* GitHub Issue: #41398

Authored-by: Jacob Wujciak-Jens <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
tolleybot pushed a commit to tmct/arrow that referenced this issue May 2, 2024
@assignUser @tolleybot
apacheGH-41398: [R][CI] Windows job failing after R 4.4 release (apac…
9bf93cf 
…he#41409)

### Rationale for this change

We can't throw warnings on cran.

### What changes are included in this PR?

Update function to match changes in libarrow added in apacheGH-39864

### Are these changes tested?

CI
### Are there any user-facing changes?

No

* GitHub Issue: apache#41398

Authored-by: Jacob Wujciak-Jens <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
raulcd pushed a commit that referenced this issue May 3, 2024
@assignUser @raulcd
GH-41398: [R][CI] Windows job failing after R 4.4 release (#41409)
e3c5060 
### Rationale for this change

We can't throw warnings on cran.

### What changes are included in this PR?

Update function to match changes in libarrow added in GH-39864

### Are these changes tested?

CI
### Are there any user-facing changes?

No

* GitHub Issue: #41398

Authored-by: Jacob Wujciak-Jens <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
vibhatha pushed a commit to vibhatha/arrow that referenced this issue May 25, 2024
@assignUser @vibhatha
apacheGH-41398: [R][CI] Windows job failing after R 4.4 release (apac…
138b794 
…he#41409)

### Rationale for this change

We can't throw warnings on cran.

### What changes are included in this PR?

Update function to match changes in libarrow added in apacheGH-39864

### Are these changes tested?

CI
### Are there any user-facing changes?

No

* GitHub Issue: apache#41398

Authored-by: Jacob Wujciak-Jens <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Cerdore Cerdore

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

Successfully merging a pull request may close this issue.

GH-39864: [C++] DataType::ToString support optionally show metadata Cerdore/arrow
2 participants
@pitrou @Cerdore