BugZilla 67646 - allow append rows to streaming workbooks #600

jlolling · 2024-02-26T14:56:07Z

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.

…com/jlolling/poi into 20230913_allow_append_for_streaming

xzel23

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.

xzel23 · 2024-02-26T14:59:37Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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


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

xzel23 · 2024-02-26T15:01:56Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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");


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

Agree, change Exception class back

xzel23 · 2024-02-26T15:02:10Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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");


same as above

Agree, change Exception class back

xzel23 · 2024-02-26T15:11:38Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

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


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?

My mistake, I will remove this code

xzel23 · 2024-02-26T15:14:31Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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");


UnsupportedOperationException

xzel23 · 2024-02-26T15:15:52Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

    }

    /**
-     * Ungroup a range of columns that were previously grouped
+     * Ungroup a range of columns that were previously groupped


please revert this line

sorry, have reverted it

xzel23 · 2024-02-26T15:17:17Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

        }
    }

    /**
-     * Ungroup a range of rows that were previously grouped
+     * Ungroup a range of rows that were previously groupped


please revert this line

sorry, have reverted it

xzel23 · 2024-02-26T15:18:13Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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)


please revert this line

sorry, have reverted it

xzel23 · 2024-02-26T15:18:45Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

     */
    @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");


UnsupportedOperationException

have changed the exception class back.

xzel23 · 2024-02-26T15:20:32Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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) {


This might break user code.

-1 this cannot be changed

However, this is compatible with the interface

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

pjfanning · 2024-02-26T16:56:38Z

this has a similar set of changes to #528

we can't accept PRs that arbitrarily change unrelated code

pjfanning · 2024-02-26T16:57:36Z

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 · 2024-02-27T08:31:31Z

Hi, There was a code breaking changes in the past! This class is not really a typical class for direct public usage. On the other side we get a huge enhancement by using the streaming workbook also in a use case where the workbook is not created from scratch and can be appended. I will write a test, this is a reasonable request. Best regards Jan Lolling

…

On 26. Feb 2024, at 17:57, PJ Fanning ***@***.***> wrote: 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 — Reply to this email directly, view it on GitHub <#600 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5KS7MWJVYN57H5TIPNGGDYVS5I3AVCNFSM6AAAAABD2LJQK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUGY2DMOBZGI>. You are receiving this because you authored the thread.

pjfanning · 2024-02-27T13:40:38Z

build.gradle

@@ -100,7 +100,7 @@ allprojects {
 //    apply plugin: 'eclipse'
    apply plugin: 'idea'

-    version = '5.2.5-SNAPSHOT'


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

pjfanning · 2024-02-27T13:41:46Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

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


use spaces not tabs

I will change my IDE settings

jlolling · 2024-02-27T13:42:58Z

Sorry, I need to build the lib for my own and changed the version number. That’s why I have changed the number back. It was not my intention to interfere your versioning.

…

On 27. Feb 2024, at 14:40, PJ Fanning ***@***.***> wrote: @pjfanning commented on this pull request. In build.gradle <#600 (comment)>: > @@ -100,7 +100,7 @@ allprojects { // apply plugin: 'eclipse' apply plugin: 'idea' - version = '5.2.5-SNAPSHOT' why can't the PR just change code related to the title of the PR? - all other stuff must be reverted — Reply to this email directly, view it on GitHub <#600 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5KS7NOLF7PFM4RGIYLPDTYVXO6DAVCNFSM6AAAAABD2LJQK2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBTGU2DAMJRGI>. You are receiving this because you authored the thread.

pjfanning · 2024-02-27T13:43:44Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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.


revert this

Sorry, forgot the java doc to fix

pjfanning · 2024-02-27T13:45:25Z

poi-scratchpad/bin/.gitignore

@@ -0,0 +1,2 @@
+/main/
+/test/


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

pjfanning · 2024-02-27T13:46:24Z

poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java

@@ -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);


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

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

…SSFRow()

pjfanning · 2024-02-27T21:30:03Z

@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 · 2024-02-27T22:01:47Z

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

jlolling added 5 commits October 9, 2023 11:27

Allow append rows in Streaming workbook

cb9e1bf

Allow append rows in Streaming workbook

335e274

become conform with ASF rules

5f1adc0

another spell error fixed

cf21581

Merge branch '20230913_allow_append_for_streaming' of https://github.…

a818af5

…com/jlolling/poi into 20230913_allow_append_for_streaming

xzel23 suggested changes Feb 26, 2024

View reviewed changes

jlolling added 2 commits February 27, 2024 14:37

Fix compile problems and removed not necessary changes

ad0b8fc

changed version number back

5c356b4

pjfanning reviewed Feb 27, 2024

View reviewed changes

Solved the internal usage of getRow() by replacing the calls to getSX…

b192eb5

…SSFRow()

		@@ -0,0 +1,2 @@
		/main/
		/test/

BugZilla 67646 - allow append rows to streaming workbooks #600

Are you sure you want to change the base?

BugZilla 67646 - allow append rows to streaming workbooks #600

Conversation

jlolling commented Feb 26, 2024

xzel23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjfanning commented Feb 26, 2024

pjfanning commented Feb 26, 2024

jlolling commented Feb 27, 2024 via email

pjfanning Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlolling commented Feb 27, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjfanning commented Feb 27, 2024

jlolling commented Feb 27, 2024

pjfanning Feb 27, 2024 •

edited

Loading