Skip to content

ARROW-7289: [C#] ListType constructor argument is redundant#5931

Closed
HashidaTKS wants to merge 2 commits intoapache:masterfrom
HashidaTKS:modify_redundant_constructor
Closed

ARROW-7289: [C#] ListType constructor argument is redundant#5931
HashidaTKS wants to merge 2 commits intoapache:masterfrom
HashidaTKS:modify_redundant_constructor

Conversation

@HashidaTKS
Copy link
Copy Markdown
Contributor

ListType constructor is redundant and should be seperated.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 2, 2019

@HashidaTKS
Copy link
Copy Markdown
Contributor Author

sorry, I'll fix the commit history.

@HashidaTKS HashidaTKS closed this Dec 2, 2019
@HashidaTKS HashidaTKS force-pushed the modify_redundant_constructor branch from c853131 to 41c9b18 Compare December 2, 2019 08:36
@HashidaTKS HashidaTKS reopened this Dec 2, 2019
@HashidaTKS
Copy link
Copy Markdown
Contributor Author

I fixed it.

@HashidaTKS HashidaTKS changed the title ARROW-7289 [C#] ListType constructor argument is redundant ARROW-7289: [C#] ListType constructor argument is redundant Dec 2, 2019
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this constructor just call into the constructor above? Similar to the way it works in C++:

arrow/cpp/src/arrow/type.h

Lines 591 to 597 in 81909dc

// List can contain any other logical value type
explicit ListType(const std::shared_ptr<DataType>& value_type)
: ListType(std::make_shared<Field>("item", value_type)) {}
explicit ListType(const std::shared_ptr<Field>& value_field) : BaseListType(type_id) {
children_ = {value_field};
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion.
I modified it.

Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It almost looks good to me.
Could you also check my comments?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we throw an exception for null valueDataType?
NullType isn't usable.

@eerhardt What do you think about this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line was removed by the new commit but same check exists in Field.
Should we throw an exception there?

https://github.com/apache/arrow/blob/master/csharp/src/Apache.Arrow/Field.cs#L44

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this behavior in either the C++ or Java implementation.

I'd say it is up to you whether to leave the Field constructor as-is, or change it to throw on a null type. I could go either way on it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. We can do it as a follow-up task when we want to do.
We can merge this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that int64 case is redundant.
Should we check both string and int64 cases?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I remove it.

takashi hashida added 2 commits December 3, 2019 13:19
* call another constructor
* remove redundant test
@HashidaTKS HashidaTKS force-pushed the modify_redundant_constructor branch from b6e0d0e to f6801dd Compare December 3, 2019 13:45
Copy link
Copy Markdown
Contributor

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks @HashidaTKS!

Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants