ARROW-5385: [Go] Implement EXTENSION datatype #10203
Conversation
|
Tagging @emkornfield @sbinet for visibility and reviews. |
zeroshade
force-pushed
the
extension-type
branch
from
18a078b
to
4dbc92c
Compare
|
The integration test seems to be failing on some rust build failure that is unrelated to this change |
|
Yap, sorry about that. We fixed it in Rust's end; if you re-trigger it should run. |
zeroshade
force-pushed
the
extension-type
branch
from
ecb8c0c
to
5621a49
Compare
|
@jorgecarleitao The tests are still failing on rust build failures :( Any ideas? |
|
Sorry will try to take a look my availability this week might be fairly limited. |
|
Well that was annoying to track down but I finally fixed the failures. oy. @emkornfield this is finally ready for review again when you have the time to do so. |
|
|
||
| // NewExtensionArrayWithStorage constructs a new ExtensionArray from the provided | ||
| // ExtensionType and uses the provided storage interface as the underlying storage. | ||
| // This will not release the storage array passed in so consumers should call Release |
There was a problem hiding this comment.
so users will generally call release right after construction?
There was a problem hiding this comment.
If they use this constructor to pass an already existing array.Interface then they still need to manually call Release on the storage array.Interface they pass in since this doesn't take ownership but instead effectively uses Retain to be another reference to the same data.
| return false | ||
| case !TypeEqual(leftField.Type, rightField.Type, opts...): | ||
| case !reflect.DeepEqual(l.index, r.index): |
There was a problem hiding this comment.
what is the scope of reflect.DeepEqual, i'm wondering if there are chances transient data might cause false negatives.
There was a problem hiding this comment.
it recursively goes through the objects to confirm that they are equal or not. if you look up at the deleted line 64 above this, I actually didn't change this logic at all despite what the diff shows. The diff isn't handling the changed order of the code well, but the previous code was already doing !reflect.DeepEqual(l.index, r.index) so there would be no change in logic from the existing setup for struct types here.
| @@ -27,7 +27,7 @@ func TestTypeEqual(t *testing.T) { | |||
| checkMetadata bool | |||
| }{ | |||
| { | |||
| nil, nil, false, false, | |||
| nil, nil, true, false, | |||
There was a problem hiding this comment.
in order for simplifying the type comparison code when handling lists / extension types I made a slight change to the logic here so that if you call TypeEqual with both arguments being nil then it returns true that they are equal. I was actually surprised when i found that this wasn't the case already such that TypeEqual(nil, nil) previously returned false. Since it simplified the code in other areas and also makes sense for nil to be equal to nil for a call to TypeEqual I didn't see an issue with making that change which only affected this one unit test which expected the false for two nils.
|
+1 thank you. |
Getting the extension metadata recognized for the integration tests with extension types also had the side effect of being a solution for the custom metadata integration tests, so i've also enabled those for Go. Closes apache#10203 from zeroshade/extension-type Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Getting the extension metadata recognized for the integration tests with extension types also had the side effect of being a solution for the custom metadata integration tests, so i've also enabled those for Go.