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.

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

ARROW-3545: [C++/Python] Use "field" terminology with StructType, specify behavior with duplicate field names #3220

Closed
wants to merge 4 commits into from

Conversation

wesm
Copy link
Member

@wesm wesm commented Dec 19, 2018

No description provided.

@kou
Copy link
Member

kou commented Dec 19, 2018

I like this.
GLib and Ruby already use "field" instead of "child": https://github.com/apache/arrow/blob/master/c_glib/arrow-glib/composite-data-type.h#L85-L97

std::shared_ptr<Field> GetChildByName(const std::string& name) const;

/// Returns -1 if name not found
ARROW_DEPRECATED("Use GetChildIndex")
Copy link
Member

@kszucs kszucs Dec 19, 2018

Choose a reason for hiding this comment

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

Suggested change
ARROW_DEPRECATED("Use GetChildIndex")
ARROW_DEPRECATED("Use GetFieldIndex")

Copy link
Member

Choose a reason for hiding this comment

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

We must apply this change. This is a typo.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1 happy with this

cdef Field child_by_name(self, name)
cpdef Field child_by_name(self, name)
cpdef Field field(self, int i)
cpdef Field field_by_name(self, name)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's useful to expose these to Python ("cpdef").

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the status quo is using __getitem__ to access the children. I'll revert this change for now (easier to add APIs later than take them away)

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

LGTM

… Define behavior of these functions when there are duplicate field names. Reflect changes in Python

Change-Id: I535de5830299d310b7721c2655a1bf5d030f284e
Change-Id: Iaab46796ae0c563cc532ecca9c1abcae86352a30
Change-Id: Ibc950474ff9edb0553cbf53597df8172dd5f288a
Change-Id: Ie79c35bdf9cb8d8ef38d520df79f36e83e0c6bf6
@wesm wesm closed this in e39e364 Dec 19, 2018
@wesm wesm deleted the ARROW-3545 branch December 20, 2018 22:51
cav71 pushed a commit to cav71/arrow that referenced this pull request Dec 22, 2018
…cify behavior with duplicate field names

Author: Wes McKinney <wesm+git@apache.org>

Closes apache#3220 from wesm/ARROW-3545 and squashes the following commits:

dc212e6 <Wes McKinney> Fix more deprecated API uses
16e1984 <Wes McKinney> Remove field_by_name/field APIs from Python bindings, cdef only
3c4abed <Wes McKinney> Fix use of deprecated APIs
2eecdbf <Wes McKinney> Rename GetChildIndex, GetChildByName for better semantic consistency. Define behavior of these functions when there are duplicate field names. Reflect changes in Python
romainfrancois pushed a commit to romainfrancois/arrow that referenced this pull request Jan 3, 2019
…cify behavior with duplicate field names

Author: Wes McKinney <wesm+git@apache.org>

Closes apache#3220 from wesm/ARROW-3545 and squashes the following commits:

dc212e6 <Wes McKinney> Fix more deprecated API uses
16e1984 <Wes McKinney> Remove field_by_name/field APIs from Python bindings, cdef only
3c4abed <Wes McKinney> Fix use of deprecated APIs
2eecdbf <Wes McKinney> Rename GetChildIndex, GetChildByName for better semantic consistency. Define behavior of these functions when there are duplicate field names. Reflect changes in Python
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.

None yet

5 participants