FieldValues: Use plain arrays instead of Vector (part 1 of 2) #66187
Conversation
This file contains 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
ryantxu
added
add to changelog
area/dataframe
breaking change
ryantxu
requested review from
tskarhed,
ashharrison90 and
academo
and removed request for
a team
leeoniya
approved these changes
Apr 7, 2023
nmarrs
approved these changes
Apr 7, 2023
|
(Open the links below in a new tab to go to the correct steps)
|
grafanabot
added
the
levitate breaking change
charandas
approved these changes
Apr 7, 2023
leeoniya
changed the title
Fix flaky unit test
…and name matchers (#65237) Co-authored-by: Brendan O'Handley <brendan.ohandley@grafana.com>
leeoniya
removed request for
charandas,
joshhunt,
a team,
zoltanbedi and
mdvictor
leeoniya
reviewed
Apr 14, 2023
| return this.buffer; | ||
| constructor(buffer?: T[]) { | ||
| super(); | ||
| this.buffer = buffer ?? []; |
There was a problem hiding this comment.
this now shallow-copies (via setter) the passed-in buffer rather than absorbing it by reference. we can't avoid this since ArrayVector now extends Array, . this new behavior required fixes in two places which had these buffer pre-allocation/mutation patterns:
- pre-allocate a buffer via
let buf = new Array(10) - wrap it with
field.values = ArrayVector(buf) - mutate the buffer expecting it to reflect in
field.valuesviabuf[i] = 100
- Create a new mutable frame:
let mutFrame = new MutableDataFrame(); - Create some field:
let field = {name: 'a', values: new ArrayVector()} - Add field:
mutFrame.addField(field)(this shallow copies field internally, and wraps .values buffers in ArrayVector) - Try to add values to the field, expecting the values to change in the mutable frame field's copy:
field.values.add("a")
leeoniya
changed the title
|
Backend code coverage report for PR #66187 |
|
Frontend code coverage report for PR #66187
|
leeoniya
reviewed
Apr 14, 2023
leeoniya
reviewed
Apr 14, 2023
Comment on lines
-62
to
-64
| expect(result.fields[0].values.toArray()).toStrictEqual([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]); | ||
| expect(result.fields[1].values.toArray()).toStrictEqual([4, null, 6, null, null, null, null, null, null, 8]); | ||
| expect(result.fields[2].values.toArray()).toStrictEqual(['a', null, 'b', null, null, null, null, null, null, 'c']); |
There was a problem hiding this comment.
NOTE: all the .toArray() removals in this PR are not essential to get the tests passing; they're just drive-by simplifications that can technically be pulled into Part 2: #66224
the key changes here are the .toStrictEqual() -> .toEqual()
torkelo
reviewed
Apr 14, 2023
torkelo
approved these changes
Apr 14, 2023
dprokop
approved these changes
Apr 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What is this feature?
This adds basic basic functions to the existing Vector interface. This will allow Vector and normal arrays to share more of the the same key types.
This is technically a breaking change because we are requiring new functions on an interface -- however it would be an extremely esoteric use case to implement the Vector interface directly rather than use one of the existing implementaions.
Why do we need this feature?
Ideally the
frame.fields[idx].valueselement would be a simple arrary... however we originally used apache arrow in the frontend, and it required an accessor function. It seemed ok at the time, but is really a mess and pretty confusing.In 10, we would like to support using values as either a simple array or Vector. This PR helps unify the typing requirements so we do not need to check
isArray(vector)all the timeWho is this feature for?
Developers
Special notes for your reviewer:
Please check that:
Release notice breaking change
Additional functions (map/filter/forEach/iterator) have been added to the root Vector interface. Any code using vectors will continue to work unchanged, but in the rare case that you have implemented Vector directly, it be missing these functions. The easiest fix is to extend FunctionalVector.
The
ArrayVectorclass now extends the native JavaScriptArrayand gains all of its prototype/instance methods as a result.