Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Cannot set column order for sheets with empty, right-side columns #1471

Closed
JohannCooper opened this issue Dec 10, 2024 · 9 comments
Closed

Comments

@JohannCooper
Copy link

JohannCooper commented Dec 10, 2024

Description

HyperFormula.setColumnOrder does not work when the right-side columns are empty.

Based on this discussion, it seems like HyperFormula.getSheetDimensions is behaving as expected. What then is the recommend way to set the column order such that the empty column is between the two non-empty columns?

The only workaround I've found is to initialize my sheet using "" instead of null.

const sheetName = "MySheet";
const engine = HyperFormula.buildFromSheets({
  [sheetName]: [["First", "=A1", ""]],
});

Video or screenshots

No response

Demo

https://codesandbox.io/p/sandbox/beautiful-hermann-pyz8z5

HyperFormula version

2.7.1

Your framework

No response

Your environment

Ubuntu 23.10

@adrianszymanski89
Copy link
Contributor

Hi @JohannCooper

Thank you for contacting us. This is a correct behavior as this error is being thrown when:

columnMapping does not define correct column permutation for some subset of columns of the given sheet

As mentioned in the documentation, this is because the null values aren't counted into the sheet width within the graph dependency.

@JohannCooper
Copy link
Author

JohannCooper commented Dec 12, 2024

Thanks for the reply @adrianszymanski89. I have two follow-ups.

  1. Returning to my original question, is my solution the recommended way "to set the column order such that the empty column is moved between the two non-empty columns"?

As mentioned in the documentation, this is because the null values aren't counted into the sheet width within the graph dependency.

  1. Can you provide a link to the documentation where it describes how null values are expected to affect a sheet's width.

@adrianszymanski89
Copy link
Contributor

@JohannCooper

It isn't stated directly in the documentation, but as the null value isn't equal to or larger than 0 or an empty string, it isn't counted as a part of the sheet's width. So in this case the correct approach would to either use 0 value or an empty string instead.

@JohannCooper
Copy link
Author

Thanks for clarifying @adrianszymanski89.

as the null value isn't equal to or larger than 0 or an empty string, it isn't counted as a part of the sheet's width

This seems to only be true if the null values appear on the right side of a sheet when the sheet is first initialized. If a sheet is initialized with null values on the left side or null values are moved to the right side after initialization, they are counted as part of the sheet’s width.

For example:

const engine = HyperFormula.buildFromSheets({
  MySheet: [[null, "First", "=B1"]],
});

// { width: 3, height: 1 }
console.log(engine.getSheetDimensions(0));

This exact case appears in this unit test.

What is unintuitive to me is that if you start with the case defined in the unit test and move the null value to the right side of its sheet, not only is it counted as part of the sheet’s width but it can also be moved using setColumnOrder unlike my original example.

const engine = HyperFormula.buildFromSheets({
  MySheet: [[null, "First", "Second"]],
});

// [ [ null, 'First', 'Second' ] ]
// { width: 3, height: 1 }
console.log(
  engine.getSheetSerialized(0),
  engine.getSheetDimensions(0)
);

// Move null value to right side of sheet
engine.setColumnOrder(0, [2, 0, 1]);

// [ [ 'First', 'Second' ] ]
// { width: 3, height: 1 }
console.log(
  engine.getSheetSerialized(0),
  engine.getSheetDimensions(0)
);

// Move null value from right side of sheet
// Same as original example but no error!
engine.setColumnOrder(0, [0, 2, 1]);

// [ [ 'First', null, 'Second' ] ]
// { width: 3, height: 1 }
console.log(
  engine.getSheetSerialized(0),
  engine.getSheetDimensions(0)
);

Demo: https://codesandbox.io/p/sandbox/hyperformula-set-column-order-bug-forked-fd9ljr?workspaceId=ws_DWrLGDSFaSWfpPsRDxaN3w

Sorry for the lengthy response. These results were unintuitive to me and I can't tell if this is a bug or expected behavior. I'd be happy to contribute to the documentation to explain this if I could get a better understanding of the expected behavior.

@adrianszymanski89
Copy link
Contributor

Hi @JohannCooper

Thank you for your thorough investigation of this problem. At this point, I think our developer responsible for Hyperformula can better answer your question.

@sequba Can I please ask you to look at this issue? Thank you.

@sequba
Copy link
Contributor

sequba commented Dec 18, 2024

Hi @JohannCooper,

Your observation about the null values is caused by the efficient memory handling in HyperFormula. For the rightmost columns and bottommost rows, the engine only puts them into the internal memory once it's necessary (e.g., they contain a non-empty value or they are referenced by some formula). Once a column/row is in the memory, it's not removed during the regular read/write operations performed on a sheet.

I can understand the confusion, as this mechanism causes sheet dimensions to change seemingly unpredictably.

What then is the recommend way to set the column order such that the empty column is between the two non-empty columns?

As you already discovered, you can write a non-empty value (e.g., "") to a column to make sure it's kept in the memory, or you can set the order of the non-empty columns and then insert the empty one to the correct position:

const sheetName = "MySheet";
const engine = HyperFormula.buildFromSheets({
  [sheetName]: [["First", "=A1"]],
});

const sheetId = engine.getSheetId(sheetName);

engine.setColumnOrder(sheetId, [0, 1]);
engine.addColumns(sheetId, [1, 1]);

console.log(engine.getSheetSerialized(sheetId));

@sequba
Copy link
Contributor

sequba commented Dec 18, 2024

Personally, I wouldn't consider it a bug, but the documentation about setColumnOrder and setRowOrder could be improved to make it clear, that the length of the permutation array must match the current sheet dimension.

@JohannCooper
Copy link
Author

@sequba

Thanks for explaining the reasoning behind the current API. Based your description, I agree that this isn't necessarily a bug.

In addition to your suggestion, it would be great to see this clarified in the documentation for getSheetDimensions as well. I'm sure others would benefit from better understanding how null values affect sheet width and height.

No further questions from my end! Thanks for all the thorough replies and for building an awesome package. Happy holidays! ✨

@sequba
Copy link
Contributor

sequba commented Dec 20, 2024

Ok. I'm closing this issue. The docs changes will be made in #1476

@sequba sequba closed this as completed Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants