Add Performance Optimizations and Bug Fix for Import Browser by cwcrystal8 · Pull Request #146 · datacommonsorg/tools

cwcrystal8 · 2022-08-25T22:51:53Z

This PR has the following changes:

Performance optimizations
- Wrote custom csvtojson method
- Removed quadratic time method
- Switch over to bulk api
- Build timedata object as you iterate originally
- Don’t make subject nodes for StatVarObservations
Bug fix for the select all/clear all button to update state properly
Updated tests for the back end changes
Fixed indentation

juliawu · 2022-09-06T19:23:56Z

-test('testing fillTemplateFromRow', () => {
-  const parser = new ParseTmcf();
-  parser.csvIndex = 8;
-  const filledTemp =
-      parser.fillTemplateFromRow(TestStr.testTMCF2, TestStr.testCSV2[0]);
-  expect(filledTemp).toBe(TestStr.expectedFilledTemp0);
-
-  // testing multiple propValues that are comma separated
-  const template =
-      'Node: dcid:test1\npropLabel1: C:TestSet->Col1, C:TestSet->Col2';
-  const row = {
-    'Col1': 'dcid:propVal1',
-    'Col2': 'dcid:propVal2',
-  };
-  const expectedMCF =
-      'Node: dcid:test1\npropLabel1: dcid:propVal1, dcid:propVal2';
-  const filledTemp2 = parser.fillTemplateFromRow(template, row);
-  expect(filledTemp2).toBe(expectedMCF);
-});
-
-test('testing csvToMCF', () => {
-  const parser = new ParseTmcf();
-  const mcf = parser.csvToMcf(TestStr.testTMCF1, TestStr.testCSV1);
-  expect(mcf).toBe(TestStr.expectedMCF1);
-});
-


For my own understanding: why are these tests being removed?

These are test cases for functions that no longer exist in the rewritten parse-tmcf file. The functions were essentially duplicates of previous parsing logic and now that the whole process is rewritten to do everything in one pass, the tests are useless.

juliawu · 2022-09-06T19:30:41Z

+    headers: {
+      'Content-Type': 'application/json',
+    },


Note to self, and the team: We'll need to add an API key to this, and all functions that use the v1 API.

Do you mean:

a. we need to add an API key in this function and pass it when making the API call - if this is the case, we want to add a TODO in the code
b. mixer needs to enforce API key for this endpoint - if this is the case, please create a github issue for this in the mixer repository

juliawu

Made a first pass, deferring approval to @beets or @chejennifer

chejennifer

nice job finding these optimizations!

chejennifer · 2022-09-14T15:23:58Z

-   * Current row number of the csv file that is being parsed.
-   * @type {number}
-   */
+ * Current row number of the csv file that is being parsed.


please revert these formatting changes

chejennifer · 2022-09-14T15:39:23Z

+  [header: string]: string | number;
 }

 interface ParsedCsvRow {


Just wondering, is it guaranteed that each csv row will only have one observation point?

i.e., are users not allowed to input a csv file where there are multiple observations in a single row like:
date,sv1,sv2
2020,1,2
2021,2,3
...

Or am I misunderstanding this data structure?

Users are allowed to input a CSV file like that but it gets parsed accordingly later on, I think that's why it's called CsvRow and not DataPoint or ObservationPoint

so there can be multiple ParsedCsvRow objects for the same row? the naming is a little confusing in that case

chejennifer · 2022-09-14T15:40:30Z

+}
+
+interface ParsedCsv {
+  otherMcfs: string[];


probably just mcfs? what is the "other" referring to?

it refers to non-StatVarObservation nodes that may have been included in the CSV file, these nodes will have MCF strings generated for them and stored here to display in the Graph Explorer later on.

then maybe something like nonSvObMcfs or something more descriptive & leave a comment

chejennifer · 2022-09-14T15:41:57Z

+
+interface ParsedTemplate {
+  dataEntities: string[];
+  otherEntities: string[];


nit: "other" is not very descriptive, maybe "nonDataEntities" or something else that actually describes what type of entities these will be

chejennifer · 2022-09-14T15:44:24Z

+ * @param {CsvRow} row The json representation of the csv row.
+ *     Each CsvRow element of the array represents one row of the csv.
+ * @return {ParsedCsvRow | null} an object containing the facet, date,
+ *     value, and other additional properties


nit: instead of "other additional properties", mention how its the mcf representation of properties for the node or something along those lines

chejennifer · 2022-09-14T16:48:18Z

-        }
-        finalReturn['localNodes'] = finalReturn['localNodes'].concat(
-            tmcfOut['localNodes'],
+        const mcfs = parsedCsv.otherMcfs;


defining this constant also feels unnecessary since it's only used in line 144 where you can just use parsedCsv.otherMcfs

chejennifer · 2022-09-14T16:51:10Z

+          const parsedMcf = mcfParser.parseMcfStr(mcf as string);
+
+          if (parsedMcf['errMsgs'].length !== 0) {
+            finalReturn['errMsgs'] = finalReturn['errMsgs'].concat({


since finalReturn['errMsgs'] is just an array, we should be able to just push the additional element like finalReturn['errMsgs'].push({...});`?

chejennifer · 2022-09-14T16:53:20Z


+type ValueObject = {
+  value: number | undefined;
+  mcf: string;


curious what this mcf is, can you add a comment?

chejennifer · 2022-09-14T16:57:42Z

-        `${dcid}`,
+        data.data.map(
+            (entity: NameResponse) =>
+            entity.values ? entity.values[0].value : entity.entity,


Empty arrays will evaluate to true, probably want to check length of entity.values before accessing the first element

chejennifer · 2022-09-14T16:59:35Z

+    body: JSON.stringify(data),
+  };
+  // response is {"data": [...list of entities...]}
+  // expected entity if dcid exists is {"entity": entity, "values":"[...]}


since you've already defined the NameResponse type, you could just say response is {"data": NameResponse[]} and don't need this whole comment

cwcrystal8 added 3 commits August 25, 2022 22:33

Add performance optimizations

c77376f

Fix select all/clear all bug

f01f676

Rebuild static files

fa88468

cwcrystal8 requested review from beets, chejennifer and juliawu August 25, 2022 22:52

cwcrystal8 marked this pull request as ready for review August 25, 2022 22:52

juliawu reviewed Sep 6, 2022

View reviewed changes

Merge branch 'master' into master

526904d

chejennifer reviewed Sep 14, 2022

View reviewed changes

Conversation

cwcrystal8 commented Aug 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliawu Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliawu left a comment

Choose a reason for hiding this comment

Uh oh!

chejennifer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juliawu Sep 6, 2022 •

edited

Loading