Refactor various aspects of arrow handling in frontend #10150

lukasmasuch · 2025-01-09T20:44:43Z

Describe your changes

This PR contains a mix of smaller refactorings across all components that use arrow. It doesn't apply any behavioral or visual changes.

Testing Plan

Doesn't apply any behavioral or visual changes
Added tests for new functions.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

raethlein · 2025-01-10T09:27:06Z

frontend/lib/src/components/elements/ArrowTable/ArrowTable.tsx

-  const allRows = range(rows)
+  const { cssId, cssStyles, caption } = table.styler ?? {}
+  const { headerRows, rows: numRows, columns: numColumns } = table.dimensions
+  const allRows = range(numRows)


nit: numRows sounds like it is an integer, whereas allRows sounds like it is an array of all actual rows. I assume it rather is allRowIndices or something like that. If that is true, I would rename the variable.

Yep, changed it 👍

raethlein · 2025-01-10T09:29:09Z

frontend/lib/src/components/elements/ArrowVegaLiteChart/arrowUtils.ts

  quiverData: Quiver | null
 ): { [field: string]: any }[] | null {
-  if (!quiverData || quiverData.data.numRows === 0) {
+  if (!quiverData || quiverData.dimensions.dataRows === 0) {


is quiverData.dimensions.dataRows a number / count? If yes, it might make sense to rename it to quiverData.dimensions.dataRowCount or similar

Yeah, I wanted to rename it a couple of times... e.g. prefix all variables with num ar suffix with Count. But haven't done so yet since its already under the dimensions object. But it probably would make code more readable if it has an additional indication that its just a number.

I updated the namings

raethlein · 2025-01-10T09:30:04Z

frontend/lib/src/components/elements/ArrowVegaLiteChart/arrowUtils.ts

+  const { dataRows: numRows, dataColumns: numColumns } = quiverData.dimensions
+
+  // This currently only works with a single index column.
+  // To support multiple index columns would require some


super-nit: // To support ... -> // Supporting ...

frontend/lib/src/components/elements/ArrowVegaLiteChart/arrowUtils.ts

frontend/lib/src/components/widgets/DataFrame/columns/NumberColumn.ts

raethlein · 2025-01-10T09:41:37Z

frontend/lib/src/components/widgets/DataFrame/columns/NumberColumn.ts


+  // If not custom format is provided & the column type is duration or period,
+  // instruct the column to use the arrow formatting for the display value.
+  const useArrowFormatting =


nit: it looks like this variable is first used in line 195. I would move it directly before that within the try block

You can see similar aspects a lot in our dataframe columns. This is for optimization. If I moved this down, it would be executed on every cell rendering -> and this is a high-risk area for performance impact. There could be a lot of cell renderings, e.g. if you scroll through a dataframe. All computations done on top here are only done once at column initialization -> so I generally try to do as many pre computations as possible that can happen on a column level on top.

ahhh the code below is in a callback?! I did not realize, thanks for pointing that out. Makes a lot of sense 👍

raethlein · 2025-01-10T09:56:12Z

frontend/lib/src/dataframes/arrowConcatUtils.ts

+  t1: PandasColumnType[],
+  t2: PandasColumnType[]
+): boolean {
+  // NOTE: We remove extra columns from the DataFrame that we add rows from.


where do we remove this? This here is a standalone function, so the comment probably needs some more context or rephrasing. I would probably write it more generically: we consider Dataframes to have the same types if all columns that exist in t1 also exist in t2 in the same order and with the same type.

I would also extend the JSDoc above to state that t2 can be larger than t1 so that it is directly obvious from the doc string.

This method was just moved out of arrowTypeUtils and it's untouched from me in its original form. While the overall addRows functionality would also require some refactoring, I do not plan to do it in the context of this project since the add_rows feature usage is almost non-existent (0.036%). It's in a ball range that we could consider deprecating/removing this feature all altogether.

I cleaned up the comments based on your suggestion

raethlein · 2025-01-10T09:57:12Z

frontend/lib/src/dataframes/arrowConcatUtils.ts

+  )
+}
+
+/** True if both arrays contain the same index types in the same order. */


In contrast to sameDataTypes, I would state in the JSDoc that If the arrays have different lengths, they are never the same

raethlein · 2025-01-10T09:58:08Z

frontend/lib/src/dataframes/arrowTypeUtils.ts

-  DatetimeIndex = "datetime",
-  Float64Index = "float64",
-  Int64Index = "int64",
  RangeIndex = "range",


If this is the only element left in the enum, maybe make it a constant? Or do we expect more elements to be added again?

Yep, only range index needs special treatment. Changed it to constant.

…tyler-usage

## Describe your changes This PR contains a mix of smaller refactorings across all components that use arrow. It doesn't apply any behavioral or visual changes. ## Testing Plan - Doesn't apply any behavioral or visual changes - Added tests for new functions. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Lukas Masuch <[email protected]>

lukasmasuch added 3 commits January 9, 2025 21:44

Clean up arrow styler usage

cfaa47d

More refactoring

eb10351

Various other refactorings

653f44e

lukasmasuch changed the title ~~[WIP] Clean up arrow styler usage~~ [WIP] Refactor arrow handling - misc Jan 9, 2025

lukasmasuch changed the title ~~[WIP] Refactor arrow handling - misc~~ [WIP] Cut down exposed methods in arrow handling API Jan 9, 2025

lukasmasuch added security-assessment-completed Security assessment has been completed for PR impact:internal PR changes only affect internal code change:refactor PR contains code refactoring without behavior change labels Jan 9, 2025

lukasmasuch and others added 19 commits January 9, 2025 23:03

Fix linting issue

f2d3d2a

Renaming

aab9f69

Fix test

c7ade11

Merge branch 'develop' into refactor/arrow-styler-usage

1fc5138

Update logic

9bd8f32

Add back export

97bf359

More cleanups

4b18032

Update test

bcb114e

Implement more type checks

d19174f

Update code

a781817

Update

ef3f378

Add imports

e12f138

Update method name

353f1ad

Update

85ee28f

Update

b4ca31f

Improve code

a5d7258

Update code

8901de5

Fix tests

bcec9bc

Fix issue

198317e

lukasmasuch changed the title ~~[WIP] Cut down exposed methods in arrow handling API~~ Refactor various aspects of arrow handling in frontend Jan 10, 2025

lukasmasuch marked this pull request as ready for review January 10, 2025 01:21

raethlein reviewed Jan 10, 2025

View reviewed changes

frontend/lib/src/components/elements/ArrowVegaLiteChart/arrowUtils.ts Outdated Show resolved Hide resolved

raethlein reviewed Jan 10, 2025

View reviewed changes

frontend/lib/src/components/widgets/DataFrame/columns/NumberColumn.ts Outdated Show resolved Hide resolved

raethlein reviewed Jan 10, 2025

View reviewed changes

lukasmasuch added 7 commits January 10, 2025 14:32

Apply feedback

bb447f7

Refactor to constant

9c7bd6b

Add todo comment

e2459c1

Add more comments

561e4fb

Update dimensions namings

c10f616

Merge remote-tracking branch 'upstream/develop' into refactor/arrow-s…

f240fa5

…tyler-usage

Rename variables

488e86e

raethlein approved these changes Jan 10, 2025

View reviewed changes

lukasmasuch added 2 commits January 10, 2025 15:53

Fix tests

25c6b56

Add test

ac32393

lukasmasuch enabled auto-merge (squash) January 10, 2025 14:57

lukasmasuch merged commit f0dacf7 into develop Jan 10, 2025
33 checks passed

lukasmasuch deleted the refactor/arrow-styler-usage branch January 10, 2025 16:01

Refactor various aspects of arrow handling in frontend #10150

Refactor various aspects of arrow handling in frontend #10150

Uh oh!

Conversation

lukasmasuch commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Testing Plan

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!

Uh oh!

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!

lukasmasuch Jan 10, 2025 • 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukasmasuch commented Jan 9, 2025 •

edited

Loading

lukasmasuch Jan 10, 2025 •

edited

Loading