Skip to content

fix: Editing issues when key columns are not first columns#2053

Merged
AkshatJawne merged 8 commits intodeephaven:mainfrom
AkshatJawne:2036_key_columns_edit_fix
Jun 7, 2024
Merged

fix: Editing issues when key columns are not first columns#2053
AkshatJawne merged 8 commits intodeephaven:mainfrom
AkshatJawne:2036_key_columns_edit_fix

Conversation

@AkshatJawne
Copy link
Copy Markdown
Contributor

Resolves #2036

Changes Implemented:

  • Changed check for key columns within range to no longer check whether the start of the range is > # of key columns and end of range is > # of key columns , but rather, iterate through and check if any of the columns are key columns

  • The check priorly was making it so that the key columns are always the first columns, which was causing issues with editing columns if they were not

@AkshatJawne AkshatJawne self-assigned this Jun 3, 2024
@mofojed mofojed removed request for bmingles and mattrunyon June 3, 2024 17:06
column <= range.endColumn;
column += 1
) {
if (this.inputTable.keyColumns.includes(this.columns[column])) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a .includes could be expensive if we have a lot of keyColumns. (It's unlikely, but we want to ensure our operations scale to thousands of columns anyway).
Also, looking at the implementation of JsInputTable.getKeyColumns (which is what is called each time we call this.inputTable.keyColumns) reveals that it's iterating through all keys each time, while also creating a new array each time: https://github.com/deephaven/deephaven-core/blob/2da295f9294031c9329b7db6a98c33bd64c3d6d6/web/client-api/src/main/java/io/deephaven/web/client/api/input/JsInputTable.java#L78
Also while reviewing this, I noticed that the isKeyColumn method is incorrect (it's checking the index against the keyColumns.length as well); as well as the isDeletable, pendingDataErrors, getCachedPendingErrors checks. We should fix these at the same time (and basically anywhere that is just checking keyColumns.length).

So I'm going to expand the scope of this ticket a little bit. Here's a few things to do:

  1. Add a method get keyColumnSet and getCachedKeyColumnSet, that just returns a Set with the key column names (not the actual column). Should be memoized by passing in table.columns and inputTable.keys (those are both stable arrays, and will not bust the memoization cache). Key lookup in a set will be much faster than doing a .includes (O(1) vs. O(n)). This would be similar to the get columnMap()/getMemoizedColumnMap functions (though returning a Set instead of a Map).
  2. isKeyColumn should use the set to check if the column is part of the set, e.g. this.keyColumnSet.has(this.columns[x].name)
  3. Everywhere we're currently looking at keyColumns.length we should probably be using the isKeyColumn method to check instead.
  4. File a ticket against deephaven-core to have JsInputTable.getKeyColumns return a frozen/stable array, similar to JsTable.getColumns. Maybe this is something you'll want to tackle as well (Java code if you're interested, I don't think you've poked the JS API yet), but it isn't strictly necessary for this ticket given the above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, will start work on implementation. In terms of working with Java code, I would love to do so, so would be very grateful for the opportunity.

@AkshatJawne AkshatJawne requested a review from mofojed June 3, 2024 20:26
@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Investigating unit test failures

Comment thread packages/iris-grid/src/IrisGridModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModel.ts
Comment thread packages/iris-grid/src/IrisGridTableModelTemplate.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModelTemplate.ts Outdated
@AkshatJawne AkshatJawne requested a review from mofojed June 6, 2024 21:39
Comment thread packages/iris-grid/src/IrisGridTableModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModel.ts
Comment thread packages/iris-grid/src/IrisGridTableModel.ts
Comment thread packages/iris-grid/src/IrisGridTableModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModel.ts Outdated
Comment thread packages/iris-grid/src/IrisGridTableModelTemplate.ts Outdated
@AkshatJawne AkshatJawne requested a review from mofojed June 7, 2024 16:02
@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Have to add check in isKeyColumn, or else, it tries to access the name property of the column at index x, and then that raises an error when the columns array is empty.

@AkshatJawne AkshatJawne merged commit 1bbcc73 into deephaven:main Jun 7, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't edit the correct cells if key columns are not the first columns

2 participants