Uh oh!
There was an error while loading. Please reload this page.
ARROW-2286: [C++/Python] Allow subscripting pyarrow.lib.StructValue - #1943
ARROW-2286: [C++/Python] Allow subscripting pyarrow.lib.StructValue#1943kszucs wants to merge 4 commits into
Conversation
pitrou
commented
Apr 25, 2018
Thanks @kszucs. Can you rebase to fix the CI failures? |
kszucs
commented
Apr 26, 2018
@pitrou rebase done, but the builds are still failing |
pitrou
commented
Apr 26, 2018
Ok, so the Travis-CI failure is unrelated (we are getting npm errors intermittently: see ARROW-2509). However, the Windows compile errors are probably caused by this PR. I'll point out the issue in a comment. |
| if index < 0: | ||
| raise KeyError(key) | ||
| return pyarrow_wrap_array(self.ap.field(index))[self.index] |
There was a problem hiding this comment.
You are passing an int64_t to a function that takes a (32-bit) int, hence the warning about truncation which is turned into an error.
pitrou
left a comment
There was a problem hiding this comment.
See my comments below about the likely cause of the Windows failure (and some more minor comments).
| std::shared_ptr<Field> GetChildByName(const std::string& name) const; | ||
| /// Returns -1 if name not found | ||
| int64_t GetChildIndex(const std::string& name) const; |
There was a problem hiding this comment.
Child indices are int everywhere else, not int64_t, so this function should return int as well.
| int64_t GetChildIndex(const std::string& name) const; | ||
| private: | ||
| mutable std::unordered_map<std::string, int> name_to_index_; |
There was a problem hiding this comment.
Add a comment that this is lazily initialized?
| std::shared_ptr<Field> result; | ||
| result = struct_type.GetChildByName("f1"); | ||
| ASSERT_TRUE(f1->Equals(result)); |
There was a problem hiding this comment.
You could actually check for pointer equality here, i.e. ASSERT_EQ(f1, result).
| ASSERT_TRUE(f3->Equals(result)); | ||
| result = struct_type.GetChildByName("not-found"); | ||
| ASSERT_TRUE(result == nullptr); |
There was a problem hiding this comment.
ASSERT_EQ will produce nicer diagnostics on failure.
| assert isinstance(set_from_array, set) | ||
| assert set_from_array == {1, 2} | ||
| def test_struct_array_subscripting(self): |
There was a problem hiding this comment.
Nit: call this test_struct_value_subscripting? You're not subscripting the array here.
No description provided.