You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by "jlolling (via GitHub)" <gi...@apache.org> on 2024/02/26 14:56:10 UTC
[PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
jlolling opened a new pull request, #600:
URL: https://github.com/apache/poi/pull/600
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on PR #600:
URL: https://github.com/apache/poi/pull/600#issuecomment-1966028389
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 <https://github.com/apache/poi/pull/600#issuecomment-1964646892>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5KS7MWJVYN57H5TIPNGGDYVS5I3AVCNFSM6AAAAABD2LJQK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUGY2DMOBZGI>.
> You are receiving this because you authored the thread.
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning closed pull request #600: BugZilla 67646 - allow append rows to streaming workbooks
URL: https://github.com/apache/poi/pull/600
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504254481
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -215,8 +216,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) {
- return _rows.get(rownum);
+ public Row getRow(int rownum) {
+ Row row = _rows.get(rownum);
+ // BugZilla 67646: allow reading all the content
+ if (row == null) {
+ row = _sh.getRow(rownum);
Review Comment:
use spaces not tabs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504252267
##########
build.gradle:
##########
@@ -100,7 +100,7 @@ allprojects {
// apply plugin: 'eclipse'
apply plugin: 'idea'
- version = '5.2.5-SNAPSHOT'
Review Comment:
why can't the PR just change code related to the title of the PR - all other stuff must be reverted
##########
build.gradle:
##########
@@ -100,7 +100,7 @@ allprojects {
// apply plugin: 'eclipse'
apply plugin: 'idea'
- version = '5.2.5-SNAPSHOT'
Review Comment:
why can't the PR just change code related to the title of the PR? - all other stuff must be reverted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504242635
##########
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)
* @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.
*/
@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");
Review Comment:
have changed the exception class back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504240541
##########
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
Review Comment:
sorry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504255495
##########
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) {
Review Comment:
ok - we might be able to change this but it would need to be in a major release - eg POI 6.0.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504261183
##########
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);
Review Comment:
Now that you've changed getRow to return `Row` - how can we just cast to SXSSFRow? Couldn't this now also be an XSSFRow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "xzel23 (via GitHub)" <gi...@apache.org>.
xzel23 commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1502753906
##########
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
Review Comment:
Why was the comment changed? "An existing" is correct.
##########
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");
Review Comment:
same as above
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -1169,7 +1180,7 @@ public boolean isDisplayRowColHeadings() {
* Breaks occur above the specified row and left of the specified column inclusive.
*
* For example, {@code sheet.setColumnBreak(2);} breaks the sheet into two parts
- * with columns A,B,C in the first and D,E,... in the second. Similar, {@code sheet.setRowBreak(2);}
+ * with columns A,B,C in the first and D,E,... in the second. Simuilar, {@code sheet.setRowBreak(2);}
Review Comment:
please revert the typo
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -1344,21 +1357,24 @@ public void groupRow(int fromRow, int toRow) {
public void setRowOutlineLevel(int rownum, int level) {
SXSSFRow row = _rows.get(rownum);
row.setOutlineLevel(level);
- setWorksheetOutlineLevelRowIfNecessary((short) Math.min(Short.MAX_VALUE, level));
+ if(level > 0 && level > outlineLevelRow) {
+ outlineLevelRow = level;
+ setWorksheetOutlineLevelRow();
+ }
}
- private void setWorksheetOutlineLevelRowIfNecessary(final short levelRow) {
+ private void setWorksheetOutlineLevelRow() {
CTWorksheet ct = _sh.getCTWorksheet();
CTSheetFormatPr pr = ct.isSetSheetFormatPr() ?
ct.getSheetFormatPr() :
ct.addNewSheetFormatPr();
- if(levelRow > _sh.getSheetFormatPrOutlineLevelRow()) {
- pr.setOutlineLevelRow(levelRow);
+ if(outlineLevelRow > 0) {
+ pr.setOutlineLevelRow((short)outlineLevelRow);
}
}
/**
- * Ungroup a range of rows that were previously grouped
+ * Ungroup a range of rows that were previously groupped
Review Comment:
please revert this line
##########
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) {
Review Comment:
This might break user code.
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -1032,7 +1043,7 @@ public void shiftRows(int startRow, int endRow, int n) {
@NotImplemented
@Override
public void shiftRows(int startRow, int endRow, int n, boolean copyRowHeight, boolean resetOriginalRowHeight) {
- throw new IllegalStateException("Not Implemented");
+ throw new RuntimeException("Not Implemented");
Review Comment:
UnsupportedOperationException
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -205,6 +206,10 @@ public void removeRow(Row row) {
return;
}
}
+ // BugZilla 67646: allow reading all the content
+ if (row.getSheet() == _sh) {
+ _sh.removeRow(row);
+ }
Review Comment:
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?
##########
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");
Review Comment:
This should rather be UnsopportedOperationException. IllegalStateException is not optimal, but we should never just throw RuntimeException.
##########
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");
Review Comment:
UnsupportedOperationException
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -1264,11 +1275,11 @@ public void setColumnGroupCollapsed(int columnNumber, boolean collapsed) {
*/
@Override
public void groupColumn(int fromColumn, int toColumn) {
- _sh.groupColumn(fromColumn, toColumn);
+ _sh.groupColumn(fromColumn,toColumn);
}
/**
- * Ungroup a range of columns that were previously grouped
+ * Ungroup a range of columns that were previously groupped
Review Comment:
please revert this line
##########
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)
Review Comment:
please revert this line
##########
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)
* @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.
*/
@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");
Review Comment:
UnsupportedOperationException
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504257169
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -215,8 +216,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) {
- return _rows.get(rownum);
+ public Row getRow(int rownum) {
+ Row row = _rows.get(rownum);
+ // BugZilla 67646: allow reading all the content
+ if (row == null) {
+ row = _sh.getRow(rownum);
Review Comment:
I will change my IDE settings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504262430
##########
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.
Review Comment:
Sorry, forgot the java doc to fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #600:
URL: https://github.com/apache/poi/pull/600#issuecomment-1967624492
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504244871
##########
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) {
Review Comment:
However, this is compatible with the interface
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504239250
##########
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");
Review Comment:
Agree, change Exception class back
##########
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");
Review Comment:
Agree, change Exception class back
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on PR #600:
URL: https://github.com/apache/poi/pull/600#issuecomment-1966580238
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 <https://github.com/apache/poi/pull/600#discussion_r1504252267>:
>
> > @@ -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 <https://github.com/apache/poi/pull/600#pullrequestreview-1903540112>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5KS7NOLF7PFM4RGIYLPDTYVXO6DAVCNFSM6AAAAABD2LJQK2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBTGU2DAMJRGI>.
> You are receiving this because you authored the thread.
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #600:
URL: https://github.com/apache/poi/pull/600#issuecomment-1964646892
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #600:
URL: https://github.com/apache/poi/pull/600#issuecomment-1964644955
this has a similar set of changes to https://github.com/apache/poi/pull/528
we can't accept PRs that arbitrarily change unrelated code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1502986484
##########
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) {
Review Comment:
-1 this cannot be changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504238538
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -205,6 +206,10 @@ public void removeRow(Row row) {
return;
}
}
+ // BugZilla 67646: allow reading all the content
+ if (row.getSheet() == _sh) {
+ _sh.removeRow(row);
+ }
Review Comment:
My mistake, I will remove this code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on PR #600:
URL: https://github.com/apache/poi/pull/600#issuecomment-1967691951
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504241527
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -1264,11 +1275,11 @@ public void setColumnGroupCollapsed(int columnNumber, boolean collapsed) {
*/
@Override
public void groupColumn(int fromColumn, int toColumn) {
- _sh.groupColumn(fromColumn, toColumn);
+ _sh.groupColumn(fromColumn,toColumn);
}
/**
- * Ungroup a range of columns that were previously grouped
+ * Ungroup a range of columns that were previously groupped
Review Comment:
sorry, have reverted it
##########
poi-ooxml/src/main/java/org/apache/poi/xssf/streaming/SXSSFSheet.java:
##########
@@ -1344,21 +1357,24 @@ public void groupRow(int fromRow, int toRow) {
public void setRowOutlineLevel(int rownum, int level) {
SXSSFRow row = _rows.get(rownum);
row.setOutlineLevel(level);
- setWorksheetOutlineLevelRowIfNecessary((short) Math.min(Short.MAX_VALUE, level));
+ if(level > 0 && level > outlineLevelRow) {
+ outlineLevelRow = level;
+ setWorksheetOutlineLevelRow();
+ }
}
- private void setWorksheetOutlineLevelRowIfNecessary(final short levelRow) {
+ private void setWorksheetOutlineLevelRow() {
CTWorksheet ct = _sh.getCTWorksheet();
CTSheetFormatPr pr = ct.isSetSheetFormatPr() ?
ct.getSheetFormatPr() :
ct.addNewSheetFormatPr();
- if(levelRow > _sh.getSheetFormatPrOutlineLevelRow()) {
- pr.setOutlineLevelRow(levelRow);
+ if(outlineLevelRow > 0) {
+ pr.setOutlineLevelRow((short)outlineLevelRow);
}
}
/**
- * Ungroup a range of rows that were previously grouped
+ * Ungroup a range of rows that were previously groupped
Review Comment:
sorry, have reverted it
##########
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)
Review Comment:
sorry, have reverted it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504257293
##########
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.
Review Comment:
revert this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504259482
##########
poi-scratchpad/bin/.gitignore:
##########
@@ -0,0 +1,2 @@
+/main/
+/test/
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "jlolling (via GitHub)" <gi...@apache.org>.
jlolling commented on code in PR #600:
URL: https://github.com/apache/poi/pull/600#discussion_r1504275708
##########
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);
Review Comment:
Agree, actually the reference to SXSSFRow is not needed here, instead Row would be fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] BugZilla 67646 - allow append rows to streaming workbooks [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #600:
URL: https://github.com/apache/poi/pull/600#issuecomment-5734746448
Superseded by #1303, which keeps the `getLastRowNum()` fix (plus `getFirstRowNum()`/`getPhysicalNumberOfRows()` and the flushed-rows case) with tests, but without the `getRow()` API change and the unrelated edits. Closing this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org