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

BugZilla 67646 - allow append rows to streaming workbooks #600

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

jlolling
Copy link

This patch works already in productive software.
It allows to read a workbook using streaming workbook and append rows to it.
The problem was the pointer to the last row was not set correctly in the SXSSFSheet.
It is fixed now and the code changes are marked with the Bugzilla ticket id.

Copy link

@xzel23 xzel23 left a comment

Choose a reason for hiding this comment

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

Changing the return type of getRow() could break user code as it is a public method in a public class, so I think this can not be approved as it is now.

IMHO, we also need a junit test for this that tests the new functionality.

There are also a couple of other things to clean up, like spelling errors introduced by the PR and directly throwing RuntimeException. I think we should not throw RuntimeException in any of these cases. I don't know if the other committers agree with the exception changes that I have proposed instead.

@@ -166,11 +167,11 @@ public SXSSFRow createRow(int rownum) {
"in the range [0," + _writer.getLastFlushedRow() + "] that is already written to disk.");
}

// attempt to overwrite an existing row in the input template
// attempt to overwrite a existing row in the input template
Copy link

Choose a reason for hiding this comment

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

Why was the comment changed? "An existing" is correct.

Copy link
Author

Choose a reason for hiding this comment

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

sorry.

@@ -1783,7 +1799,7 @@ public CellRange<? extends Cell> setArrayFormula(String formula, CellRangeAddres
// corrupted .xlsx files as rows appear multiple times in the resulting sheetX.xml files
// return _sh.setArrayFormula(formula, range);

throw new IllegalStateException("Not Implemented");
throw new RuntimeException("Not Implemented");
Copy link

Choose a reason for hiding this comment

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

This should rather be UnsopportedOperationException. IllegalStateException is not optimal, but we should never just throw RuntimeException.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, change Exception class back

@@ -1798,7 +1814,7 @@ public CellRange<? extends Cell> removeArrayFormula(Cell cell) {
// corrupted .xlsx files as rows appear multiple times in the resulting sheetX.xml files
// return _sh.removeArrayFormula(cell);

throw new IllegalStateException("Not Implemented");
throw new RuntimeException("Not Implemented");
Copy link

Choose a reason for hiding this comment

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

same as above

Copy link
Author

Choose a reason for hiding this comment

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

Agree, change Exception class back

// BugZilla 67646: allow reading all the content
if (row.getSheet() == _sh) {
_sh.removeRow(row);
}
Copy link

Choose a reason for hiding this comment

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

I don't understand the intent here. row.getSheet() is checked right at the start of the method. And the row is removed in the loop. AFAIK the row is not updated when that happens, so row.getSheet() will still return this. So what should happen here?

Copy link
Author

Choose a reason for hiding this comment

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

My mistake, I will remove this code

@@ -1008,7 +1019,7 @@ public boolean getForceFormulaRecalculation() {
@NotImplemented
@Override
public void shiftRows(int startRow, int endRow, int n) {
throw new IllegalStateException("Not Implemented");
throw new RuntimeException("Not Implemented");
Copy link

Choose a reason for hiding this comment

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

UnsupportedOperationException

}

/**
* Ungroup a range of columns that were previously grouped
* Ungroup a range of columns that were previously groupped
Copy link

Choose a reason for hiding this comment

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

please revert this line

Copy link
Author

Choose a reason for hiding this comment

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

sorry, have reverted it

}
}

/**
* Ungroup a range of rows that were previously grouped
* Ungroup a range of rows that were previously groupped
Copy link

Choose a reason for hiding this comment

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

please revert this line

Copy link
Author

Choose a reason for hiding this comment

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

sorry, have reverted it

@@ -1373,33 +1389,33 @@ public void ungroupRow(int fromRow, int toRow) {
*
* <i>Not implemented for expanding (i.e. collapse == false)</i>
*
* @param row start row of a grouped range of rows (0-based)
* @param row start row of a groupped range of rows (0-based)
Copy link

Choose a reason for hiding this comment

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

please revert this line

Copy link
Author

Choose a reason for hiding this comment

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

sorry, have reverted it

*/
@Override
public void setRowGroupCollapsed(int row, boolean collapse) {
if (collapse) {
collapseRow(row);
} else {
//expandRow(rowIndex);
throw new IllegalStateException("Unable to expand row: Not Implemented");
throw new RuntimeException("Unable to expand row: Not Implemented");
Copy link

Choose a reason for hiding this comment

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

UnsupportedOperationException

Copy link
Author

Choose a reason for hiding this comment

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

have changed the exception class back.

@@ -215,8 +220,13 @@ public void removeRow(Row row) {
* @return Row representing the rownumber or null if its not defined on the sheet
*/
@Override
public SXSSFRow getRow(int rownum) {
Copy link

Choose a reason for hiding this comment

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

This might break user code.

Copy link
Contributor

Choose a reason for hiding this comment

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

-1 this cannot be changed

Copy link
Author

Choose a reason for hiding this comment

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

However, this is compatible with the interface

Copy link
Contributor

Choose a reason for hiding this comment

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

ok - we might be able to change this but it would need to be in a major release - eg POI 6.0.0

@pjfanning
Copy link
Contributor

this has a similar set of changes to #528

we can't accept PRs that arbitrarily change unrelated code

@pjfanning
Copy link
Contributor

Changing the return type of getRow() could break user code as it is a public method in a public class, so I think this can not be approved as it is now.

IMHO, we also need a junit test for this that tests the new functionality.

There are also a couple of other things to clean up, like spelling errors introduced by the PR and directly throwing RuntimeException. I think we should not throw RuntimeException in any of these cases. I don't know if the other committers agree with the exception changes that I have proposed instead.

if there is no test - then we shouldn't merge

@jlolling
Copy link
Author

jlolling commented Feb 27, 2024 via email

build.gradle Outdated
@@ -100,7 +100,7 @@ allprojects {
// apply plugin: 'eclipse'
apply plugin: 'idea'

version = '5.2.5-SNAPSHOT'
Copy link
Contributor

@pjfanning pjfanning Feb 27, 2024

Choose a reason for hiding this comment

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

why can't the PR just change code related to the title of the PR? - all other stuff must be reverted

Row row = _rows.get(rownum);
// BugZilla 67646: allow reading all the content
if (row == null) {
row = _sh.getRow(rownum);
Copy link
Contributor

Choose a reason for hiding this comment

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

use spaces not tabs

Copy link
Author

Choose a reason for hiding this comment

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

I will change my IDE settings

@jlolling
Copy link
Author

jlolling commented Feb 27, 2024 via email

@@ -1375,7 +1387,7 @@ public void ungroupRow(int fromRow, int toRow) {
*
* @param row start row of a grouped range of rows (0-based)
* @param collapse whether to expand/collapse the detail rows
* @throws IllegalStateException if collapse is false as this is not implemented for SXSSF.
* @throws RuntimeException if collapse is false as this is not implemented for SXSSF.
Copy link
Contributor

Choose a reason for hiding this comment

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

revert this

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, forgot the java doc to fix

@@ -0,0 +1,2 @@
/main/
/test/
Copy link
Contributor

Choose a reason for hiding this comment

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

all these gitignore changes should be removed - you can update your own user .gitignore but we don't need these in svn - POI is not a Git project - it is an svn project

@@ -1431,12 +1443,12 @@ private int findStartOfRowOutlineGroup(int rowIndex) {

private int writeHidden(SXSSFRow xRow, int rowIndex) {
int level = xRow.getOutlineLevel();
SXSSFRow currRow = getRow(rowIndex);
SXSSFRow currRow = (SXSSFRow) getRow(rowIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you've changed getRow to return Row - how can we just cast to SXSSFRow? Couldn't this now also be an XSSFRow?

Copy link
Author

Choose a reason for hiding this comment

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

Agree, actually the reference to SXSSFRow is not needed here, instead Row would be fine.

@pjfanning
Copy link
Contributor

@jlolling can we pause this PR for a few days? I'm concerned that we have no justification for this change - no use case defined by you - no test cases for us to think through.

The code today is deliberate. It is not an oversight to not support appending rows in SXSSF code base. SXSSF code is mainly for writing fresh workbooks with less memory overhead than the XSSF code. The idea that you can provide an XSSFWorkbook as a template is about allowing styles and other metadata to be provided.

Supporting appends to existing sheets gets us into a slippery slope to trying to support editing of existing sheets. Frankly, without justification for this change, I would say why not use XSSFWorkbook?

I'm not against appends as a general rule but I would like to concentrate on very specific use cases and then carefully document what is supported and what is not.

It may be possible to support a very precise append scenario that does not involve us changing the getRow API. The changes you have made in this PR look to have big impacts. We may be able to support your use case with much less change.

Frankly, you should never start by giving us a random PR with no context - especially one with so many unwanted changes to exceptions and git styles (etc.) - so much extraneous unnecessary fluff that I can't yet work out what it is you want to do.

@jlolling
Copy link
Author

Hi pjfanning,

yes of course. It is not my intention to gain all attention. I will write testcases and will explain better what is my intention for this change.
Best regards
Jan

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

Successfully merging this pull request may close these issues.

3 participants