ARROW-5762: [JS] Align Map type impl with the spec #5371
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheNeuralBit
approved these changes
Sep 13, 2019
Member
TheNeuralBit
left a comment
TheNeuralBit
left a comment
There was a problem hiding this comment.
👍 LGTM other than the CI failure.. looks like there's an issue with ordering in es2015-umd
| return row; | ||
| }; | ||
| })(); | ||
|
|
Member
There was a problem hiding this comment.
Could you run the performance tests to make sure this change doesn't affect perf for the row proxies? I don't think it would, but doesn't hurt to make sure.
| function generateMap<T extends Map_>(this: TestDataVectorGenerator, type: T, length = 100, nullCount = length * 0.2 | 0, children = type.children.map((f) => this.visit(f.type, length, nullCount))): GeneratedVector<V<T>> { | ||
| const vecs = children.map(({ vector }) => vector); | ||
| const cols = children.map(({ values }) => values); | ||
| function generateMap<T extends Map_>(this: TestDataVectorGenerator, |
Member
There was a problem hiding this comment.
What do you think about adding a simple test for MapVector to vector-tests.ts as well? The generated data tests are nice for completeness, but I like that vector-tests serve as a form of documentation and a sanity check in case anything goes wrong in this test
trxcllnt
force-pushed
the
ARROW-5762/map-type
branch
from
2cd461d to
fbde391
Compare
trxcllnt
force-pushed
the
ARROW-5762/map-type
branch
from
fbde391 to
3a0eca9
Compare
Codecov Report
@@ Coverage Diff @@
## master #5371 +/- ##
===========================================
+ Coverage 87.57% 89.56% +1.98%
===========================================
Files 1041 102 -939
Lines 151814 6603 -145211
Branches 1447 1495 +48
===========================================
- Hits 132958 5914 -127044
+ Misses 18492 678 -17814
+ Partials 364 11 -353
Continue to review full report at Codecov.
|
pprudhvi
pushed a commit
to pprudhvi/arrow
that referenced
this pull request
Sep 16, 2019
This PR closes [ARROW-5762](https://issues.apache.org/jira/browse/ARROW-5762) by aligning the Map implementation with the spec, enabling its inclusion in the integration tests. Feature-wise, the biggest change is to the `Struct` and `Map` rows. `Row` is now an abstract base class extended by `StructRow` and `MapRow`. Row implements [JS's native Map](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map) interface (and will pass `row instanceof Map` checks). To align with JS Maps, the `Symbol.iterator` return type changes from `IterableIterator<TValue>` to `IterableIterator<[TKey, TValue]>`. `Struct` fields are uniform and known in advance, whereas `Map` fields vary by row. The `StructRow` will still take advantage of the `Object.create()` technique to create fast instances using a single `StructRow` instance as its prototype. However `MapRow` must either be dynamic or have its fields defined on construction, so I've changed `MapRow` to return an ES6 Proxy if available ([supported everywhere except IE11](https://caniuse.com/#search=Proxy)) and fall back to `Object.defineProperties()` if not. Closes apache#5371 from trxcllnt/ARROW-5762/map-type and squashes the following commits: 3a0eca9 <ptaylor> ensure generated strings are unique da2a1ed <ptaylor> Merge branch 'master' into ARROW-5762/map-type 58f244a <ptaylor> reimplement Map type Authored-by: ptaylor <paul.e.taylor@me.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR closes ARROW-5762 by aligning the Map implementation with the spec, enabling its inclusion in the integration tests.
Feature-wise, the biggest change is to the
StructandMaprows.Rowis now an abstract base class extended byStructRowandMapRow.Row implements JS's native Map interface (and will pass
row instanceof Mapchecks). To align with JS Maps, theSymbol.iteratorreturn type changes fromIterableIterator<TValue>toIterableIterator<[TKey, TValue]>.Structfields are uniform and known in advance, whereasMapfields vary by row. TheStructRowwill still take advantage of theObject.create()technique to create fast instances using a singleStructRowinstance as its prototype.However
MapRowmust either be dynamic or have its fields defined on construction, so I've changedMapRowto return an ES6 Proxy if available (supported everywhere except IE11) and fall back toObject.defineProperties()if not.