plainheart commented on code in PR #21699:
URL: https://github.com/apache/echarts/pull/21699#discussion_r3609868586
##########
src/data/Source.ts:
##########
@@ -401,12 +401,23 @@ function determineSourceDimensions(
}
function objectRowsCollectDimensions(data: OptionSourceDataObjectRows):
DimensionDefinitionLoose[] {
- let firstIndex = 0;
- let obj;
- while (firstIndex < data.length && !(obj = data[firstIndex++])) {} //
jshint ignore: line
- if (obj) {
- return keys(obj);
+ const dimensionNameMap = createHashMap<true, DimensionName>();
+ const dimensionsDefine: DimensionDefinitionLoose[] = [];
+
+ for (let i = 0; i < data.length; i++) {
+ const obj = data[i];
+ if (!obj) {
+ continue;
+ }
+ each(keys(obj), function (name) {
+ if (!dimensionNameMap.get(name)) {
+ dimensionNameMap.set(name, true);
+ dimensionsDefine.push(name);
+ }
+ });
}
Review Comment:
Thanks for fixing this issue. I agree that collecting keys from all object
rows is necessary to handle sparse fields correctly, but it also adds an
`O(total number of object keys)` pre-scan during source initialization when
`dimensions` is not explicitly provided.
Could you please add a small performance check to verify that this does not
introduce a noticeable overhead for large datasets? It would be useful to
compare:
- the previous behavior (reading only the first non-empty object)
- the new full-scan behavior
- the path where `dataset.dimensions` is explicitly provided
For example, testing with around 100k rows would be helpful, covering both
stable object shapes and sparse keys that appear only in later rows. Measuring
`setOption` or source creation time should be sufficient. This would help us
assess the practical impact of the additional scan.
Also, the current implementation creates a `keys(obj)` array and uses a
callback for every row. If this shows up in the benchmark, could we avoid the
intermediate allocation and callback overhead with something like:
```ts
for (const name in obj) {
if (hasOwn(obj, name) && !dimensionNameMap.get(name)) {
dimensionNameMap.set(name, true);
dimensionsDefine.push(name);
}
}
```
This preserves the intended semantics, collecting every field while
retaining first-seen order, while potentially reducing allocations and per-row
overhead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
