Bug 43901

Summary: Last cell number in row is set incorrectly
Product: POI Reporter: Richard Evans <richard.evans>
Component: HSSFAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: major CC: Bina, info
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Hardware: Other   
OS: other   
Attachments: test case demonstrating the problem
Extended test case with the case reporetd in bug 43901
svn diff of changes to HSSFRow and test classes

Description Richard Evans 2007-11-19 04:12:37 UTC
If you write an XLS file using POI, and then read it back,
HSSFRow.getLastCellNum() returns the wrong value.

For example, if you write three columns to the file, and then read it back,
getLastCellNum() returns 2, not 3.  The method should return the last cell
number PLUS 1.

The problem is that RowRecord.setLastCol() is called incorrectly in
HSSFRow.addCell.  It is called with the last cell number, not the last cell
number PLUS 1.

This seems to be exactly the symptom reported in bug 14890, which was rejected
as an error in the test code.

See attached test case.
Comment 1 Richard Evans 2007-11-19 04:15:16 UTC
Created attachment 21149 [details]
test case demonstrating the problem

The test case uses POI to write a simple 3x3 xls file and then read it back. 
getLastCellNum on each row in the re-read file returns the wrong value.
Comment 2 Paolo Mottadelli 2008-01-09 02:58:13 UTC
Created attachment 21364 [details]
Extended test case with the case reporetd in bug 43901

Notice that cellNum is '0' base. So if you create 3 cells, the lastCellNum has
to be 2.
I extended the testcase just to include the reported case.
Comment 3 Nick Burch 2008-01-09 03:13:22 UTC
Patch from Paolo to the test applied to svn

I agree with Paolo, this bug report is invalid, and POI is working as expected
Comment 4 Richard Evans 2008-01-09 03:25:36 UTC
I'm sorry to disagree, but the Javadoc for HSSFRow.getLastCellNum() says:

public short getLastCellNum()

    gets the number of the last cell contained in this row PLUS ONE.

    Returns:
        short representing the last logical cell in the row PLUS ONE, or -1 if
the row does not contain any cells.

I understand that cell numbers are zero based, but the documentation does say
PLUS ONE.  So if there are three cells, with numbers 0, 1 and 2, I would expect
that getLastCellNum() should return 3, not 2.

If you read a three-column XLS file created in Excel, you DO get 3 as the result
of getLastCellNum().
Comment 5 Richard Evans 2008-03-04 07:51:58 UTC
I still believe this is a bug.  Has it been fixed in the latest release?
Comment 6 Josh Micich 2008-03-06 08:31:18 UTC
(In reply to comment #5)
> I still believe this is a bug.  Has it been fixed in the latest release?


I admit I had to read the javadoc twice before I understood what this method did, but I am convinced the behaviour is correct and intentional.


This method allows standard 'zero based' array iteration (i=0; i<max; i++) over cells in a row.  I have used getLastCellNum() like this, POI internally uses getLastCellNum() like this, and I'm sure many other POI dependent projects have done the same.  So it's very unlikely that the behaviour can be changed.

The javadoc could be improved a bit. Here is a suggestion:

    /**
     * Gets the index of the last cell contained in this row <b>PLUS ONE</b>.  The result also 
     * happens to be the 1-based column number of the last cell.  This value can be used as a
     * standard upper bound when iterating over cells:
     * <pre> 
     * int maxColIx = row.getLastCellNum();
     * for(short colIx=0; colIx&lt;maxColIx; colIx++) {
     *   HSSFCell cell = row.getCell(colIx);
     *   if(cell == null) {
     *     continue;
     *   }
     *   //... do something with cell
     * }
     * </pre>
     * 
     * @return short representing the last logical cell in the row <b>PLUS ONE</b>, or -1 if the
     *  row does not contain any cells.
     */


I'm going to mark this bug as WORKSFORME.  If you still think there is a bug, could you please upload a junit test that shows it.
Comment 7 Richard Evans 2008-03-06 08:46:35 UTC
I agree with the functionality of getLastCellNum().  The specification is fine for me.

However the original report was that XLS files _written_ by POI have the wrong value for getLastCellNum().  If you write an XLS with three columns, then read it back, getLastCellNum() returns 2 not 3.  

If you read a XLS file with three columns generated by Excel, getLastCellNum() returns 3 as expected.

See the original attachment with this issue.

Our temporary workround was to modify HSSFRow.addCell to read:

    /**
     * used internally to add a cell.
     */

    private void addCell(HSSFCell cell)
    {
        short column=cell.getCellNum();
        if (row.getFirstCol() == -1)
        {
            row.setFirstCol(column);
        }
        if (row.getLastCol() == -1)
        {
            row.setLastCol((short) (column + 1));
        }

        if(column>=cells.length)
        {
          HSSFCell[] oldCells=cells;
          int newSize=oldCells.length*2;
          if(newSize<column+1) newSize=column+1;
          cells=new HSSFCell[newSize];
          System.arraycopy(oldCells,0,cells,0,oldCells.length);
        }
        cells[column]=cell;

        if (column < row.getFirstCol())
        {
            row.setFirstCol(column);
        }
        if (column >= row.getLastCol())
        {
            row.setLastCol((short) (column + 1));
        }
    }
Comment 8 Josh Micich 2008-03-06 11:33:44 UTC
(In reply to comment #7)
> I agree with the functionality of getLastCellNum().  The specification is fine
> for me.
> However the original report was that XLS files _written_ by POI have the wrong
> value for getLastCellNum().  If you write an XLS with three columns, then read
> it back, getLastCellNum() returns 2 not 3.  

You are right. Interestingly, if you re-save that file from Excel, getLastCellNum() returns 3, as expected.  So something is definitely wrong.

To reproduce the bug you don't even need to write-out/read-back the workbook.  You can just call getLastCellNum() after adding a cell:

    public void testLastCellNumIsCorrectAfterAddCell_bug43901(){
        HSSFWorkbook book = new HSSFWorkbook();
        HSSFSheet sheet = book.createSheet("test");
        HSSFRow row = sheet.createRow(0);

        // New row has last col -1
        assertEquals(-1, row.getLastCellNum());
        if(row.getLastCellNum() == 0) {
            fail("Identified bug 43901");
        }

        // Create two cells, will return one higher
        //  than that for the last number
        row.createCell((short) 0);
        assertEquals(1, row.getLastCellNum());
        row.createCell((short) 255);
        assertEquals(256, row.getLastCellNum());
    }

// 
It seems like there are a few related problems ( moving and removing cells too).  I'll upload a patch shortly to fix all of these.
Comment 9 Josh Micich 2008-03-06 11:42:27 UTC
Created attachment 21643 [details]
svn diff of changes to HSSFRow and test classes
Comment 10 Nick Burch 2008-03-07 03:17:01 UTC
Thanks for the patch Josh, applied to svn
Comment 11 Yegor Kozlov 2008-05-05 07:49:49 UTC
*** Bug 43015 has been marked as a duplicate of this bug. ***
Comment 12 Josh Micich 2008-05-10 20:17:11 UTC
*** Bug 33317 has been marked as a duplicate of this bug. ***