diff options
| -rw-r--r-- | src/changes/changes.xml | 4 | ||||
| -rw-r--r-- | src/java/com/healthmarketscience/jackcess/Index.java | 80 | ||||
| -rw-r--r-- | src/java/com/healthmarketscience/jackcess/Table.java | 78 | ||||
| -rw-r--r-- | test/src/java/com/healthmarketscience/jackcess/DatabaseTest.java | 27 | ||||
| -rw-r--r-- | test/src/java/com/healthmarketscience/jackcess/IndexTest.java | 40 |
5 files changed, 185 insertions, 44 deletions
diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 11d8e23..b92065b 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -6,6 +6,10 @@ </properties> <body> <release version="1.1.10" date="TBD"> + <action dev="jahlborn" type="fix" issue="1681954"> + Update table row count correctly on row deletion or bulk row addition, + bug #1681954. + </action> <action dev="jahlborn" type="update" issue="1565216"> Add experimental support for auto-number columns, feature request #1565216. diff --git a/src/java/com/healthmarketscience/jackcess/Index.java b/src/java/com/healthmarketscience/jackcess/Index.java index 0cd6689..073ce57 100644 --- a/src/java/com/healthmarketscience/jackcess/Index.java +++ b/src/java/com/healthmarketscience/jackcess/Index.java @@ -32,6 +32,7 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.nio.ByteOrder; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Comparator; @@ -42,7 +43,10 @@ import java.util.List; import java.util.Map; import java.util.SortedSet; import java.util.TreeSet; + import org.apache.commons.lang.builder.CompareToBuilder; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; @@ -52,6 +56,8 @@ import org.apache.commons.lang.builder.CompareToBuilder; */ public class Index implements Comparable<Index> { + private static final Log LOG = LogFactory.getLog(Index.class); + /** Max number of columns in an index */ private static final int MAX_COLUMNS = 10; @@ -182,7 +188,9 @@ public class Index implements Comparable<Index> { /** Page number of the index data */ private int _pageNumber; private int _parentPageNumber; - /** Number of rows in the index */ + /** Number of rows in the index + NOTE: this does not actually seem to be the row count, unclear what the + value means*/ private int _rowCount; private JetFormat _format; private SortedSet<Entry> _entries; @@ -317,9 +325,7 @@ public class Index implements Comparable<Index> { buffer.put((byte) 0); //Unknown byte[] entryMask = new byte[_format.SIZE_INDEX_ENTRY_MASK]; int totalSize = 0; - Iterator iter = _entries.iterator(); - while (iter.hasNext()) { - Entry entry = (Entry) iter.next(); + for(Entry entry : _entries) { int size = entry.size(); totalSize += size; int idx = totalSize / 8; @@ -330,9 +336,7 @@ public class Index implements Comparable<Index> { entryMask[idx] |= (1 << (totalSize % 8)); } buffer.put(entryMask); - iter = _entries.iterator(); - while (iter.hasNext()) { - Entry entry = (Entry) iter.next(); + for(Entry entry : _entries) { entry.write(buffer); } buffer.putShort(2, (short) (_format.PAGE_SIZE - buffer.position())); @@ -498,7 +502,7 @@ public class Index implements Comparable<Index> { /** - * Add a row to this index + * Adds a row to this index * <p> * Forces index initialization. * @@ -511,10 +515,48 @@ public class Index implements Comparable<Index> { { // make sure we've parsed the entries initialize(); - + + ++_rowCount; _entries.add(new Entry(row, pageNumber, rowNumber)); } + /** + * Removes a row from this index + * <p> + * Forces index initialization. + * + * @param row Row to remove + * @param pageNumber Page number on which the row is removed + * @param rowNumber Row number at which the row is removed + */ + public void deleteRow(Object[] row, int pageNumber, byte rowNumber) + throws IOException + { + // make sure we've parsed the entries + initialize(); + + --_rowCount; + Entry oldEntry = new Entry(row, pageNumber, rowNumber); + if(!_entries.remove(oldEntry)) { + // the caller may have only read some of the row data, if this is the + // case, just search for the page/row numbers + boolean removed = false; + for(Iterator<Entry> iter = _entries.iterator(); iter.hasNext(); ) { + Entry entry = iter.next(); + if((entry.getPage() == pageNumber) && + (entry.getRow() == rowNumber)) { + iter.remove(); + removed = true; + break; + } + } + if(!removed) { + LOG.warn("Failed removing index entry " + oldEntry + " for row: " + + Arrays.asList(row)); + } + } + } + @Override public String toString() { StringBuilder rtn = new StringBuilder(); @@ -719,7 +761,7 @@ public class Index implements Comparable<Index> { return new FixedEntryColumn(col); } - public List getEntryColumns() { + public List<EntryColumn> getEntryColumns() { return _entryColumns; } @@ -733,9 +775,8 @@ public class Index implements Comparable<Index> { public int size() { int rtn = 4; - Iterator iter = _entryColumns.iterator(); - while (iter.hasNext()) { - rtn += ((EntryColumn) iter.next()).size(); + for(EntryColumn entryCol : _entryColumns) { + rtn += entryCol.size(); } return rtn; } @@ -744,9 +785,8 @@ public class Index implements Comparable<Index> { * Write this entry into a buffer */ public void write(ByteBuffer buffer) throws IOException { - Iterator iter = _entryColumns.iterator(); - while (iter.hasNext()) { - ((EntryColumn) iter.next()).write(buffer); + for(EntryColumn entryCol : _entryColumns) { + entryCol.write(buffer); } buffer.put((byte) (_page >>> 16)); buffer.put((byte) (_page >>> 8)); @@ -763,15 +803,15 @@ public class Index implements Comparable<Index> { if (this == other) { return 0; } - Iterator myIter = _entryColumns.iterator(); - Iterator otherIter = other.getEntryColumns().iterator(); + Iterator<EntryColumn> myIter = _entryColumns.iterator(); + Iterator<EntryColumn> otherIter = other.getEntryColumns().iterator(); while (myIter.hasNext()) { if (!otherIter.hasNext()) { throw new IllegalArgumentException( "Trying to compare index entries with a different number of entry columns"); } - EntryColumn myCol = (EntryColumn) myIter.next(); - EntryColumn otherCol = (EntryColumn) otherIter.next(); + EntryColumn myCol = myIter.next(); + EntryColumn otherCol = otherIter.next(); int i = myCol.compareTo(otherCol); if (i != 0) { return i; diff --git a/src/java/com/healthmarketscience/jackcess/Table.java b/src/java/com/healthmarketscience/jackcess/Table.java index 3bed3cb..6acab8d 100644 --- a/src/java/com/healthmarketscience/jackcess/Table.java +++ b/src/java/com/healthmarketscience/jackcess/Table.java @@ -30,6 +30,7 @@ package com.healthmarketscience.jackcess; import java.io.IOException; import java.nio.ByteBuffer; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Comparator; @@ -161,7 +162,7 @@ public class Table readTableDefinition(tableBuffer); tableBuffer = null; - _rowState = new RowState(true); + _rowState = new RowState(true, _maxColumnCount); } /** @@ -236,13 +237,29 @@ public class Table // FIXME, want to make this static, but writeDataPage is not static, also, this may screw up other rowstates... + // see if row was already deleted + if(_rowState.isDeleted()) { + throw new IllegalStateException("Deleting already deleted row"); + } + // delete flag always gets set in the "root" page (even if overflow row) ByteBuffer rowBuffer = _rowState.getPage(_pageChannel); - int index = getRowStartOffset(_currentRowInPage, _format); - rowBuffer.putShort(index, (short)(rowBuffer.getShort(index) + int pageNumber = _rowState.getPageNumber(); + int rowIndex = getRowStartOffset(_currentRowInPage, _format); + rowBuffer.putShort(rowIndex, (short)(rowBuffer.getShort(rowIndex) | DELETED_ROW_MASK | OVERFLOW_ROW_MASK)); - writeDataPage(rowBuffer, _rowState.getPageNumber()); + writeDataPage(rowBuffer, pageNumber); _rowState.setDeleted(true); + + // update the indexes + for(Index index : _indexes) { + index.deleteRow(_rowState.getRowValues(), pageNumber, + (byte)_currentRowInPage); + } + + // make sure table def gets updated + --_rowCount; + updateTableDefinition(); } /** @@ -265,7 +282,7 @@ public class Table return null; } - return getRow(rowBuffer, getRowNullMask(rowBuffer), _columns, + return getRow(_rowState, rowBuffer, getRowNullMask(rowBuffer), _columns, columnNames); } @@ -312,7 +329,7 @@ public class Table return null; } - return getRow(rowBuffer, getRowNullMask(rowBuffer), columns, + return getRow(rowState, rowBuffer, getRowNullMask(rowBuffer), columns, columnNames); } @@ -320,6 +337,7 @@ public class Table * Reads the row data from the given row buffer. Leaves limit unchanged. */ private static Map<String, Object> getRow( + RowState rowState, ByteBuffer rowBuffer, NullMask nullMask, Collection<Column> columns, @@ -329,10 +347,18 @@ public class Table Map<String, Object> rtn = new LinkedHashMap<String, Object>( columns.size()); for(Column column : columns) { + Object value = null; if((columnNames == null) || (columnNames.contains(column.getName()))) { // Add the value to the row data - rtn.put(column.getName(), getRowColumn(rowBuffer, nullMask, column)); + value = getRowColumn(rowBuffer, nullMask, column); + rtn.put(column.getName(), value); } + + // cache the row values in order to be able to update the index on row + // deletion. note, most of the returned values are immutable, except + // for binary data (returned as byte[]), but binary data shouldn't be + // indexed anyway. + rowState._rowValues[column.getColumnNumber()] = value; } return rtn; @@ -1033,26 +1059,40 @@ public class Table dataPage.put(rowData[i]); // update the indexes - Iterator<Index> indIter = _indexes.iterator(); - while (indIter.hasNext()) { - Index index = indIter.next(); - index.addRow(rows.get(i), pageNumber, (byte) rowNum); + for(Index index : _indexes) { + index.addRow(rows.get(i), pageNumber, (byte)rowNum); } } writeDataPage(dataPage, pageNumber); //Update tdef page + _rowCount += rows.size(); + updateTableDefinition(); + } + + /** + * Updates the table definition after rows are modified. + */ + private void updateTableDefinition() throws IOException + { + // load table definition ByteBuffer tdefPage = _pageChannel.createPageBuffer(); _pageChannel.readPage(tdefPage, _tableDefPageNumber); - tdefPage.putInt(_format.OFFSET_NUM_ROWS, ++_rowCount); + + // make sure rowcount and autonumber are up-to-date + tdefPage.putInt(_format.OFFSET_NUM_ROWS, _rowCount); tdefPage.putInt(_format.OFFSET_NEXT_AUTO_NUMBER, _lastAutoNumber); + + // write any index changes Iterator<Index> indIter = _indexes.iterator(); for (int i = 0; i < _indexes.size(); i++) { tdefPage.putInt(_format.OFFSET_INDEX_DEF_BLOCK + - i * _format.SIZE_INDEX_DEFINITION + 4, _rowCount); + (i * _format.SIZE_INDEX_DEFINITION) + 4, _rowCount); Index index = indIter.next(); index.update(); } + + // write modified table definition _pageChannel.writePage(tdefPage, _tableDefPageNumber); } @@ -1393,15 +1433,19 @@ public class Table /** the row buffer which contains the final data (after following any overflow pointers) */ public ByteBuffer _finalRowBuffer; - - public RowState(boolean hardRowBuffer) { + /** values read from the last row */ + public Object[] _rowValues; + + public RowState(boolean hardRowBuffer, int colCount) { _rowBufferH = TempPageHolder.newHolder(hardRowBuffer); + _rowValues = new Object[colCount]; } public void reset() { _finalRowBuffer = null; _deleted = false; _overflow = false; + Arrays.fill(_rowValues, null); } public int getPageNumber() { @@ -1430,6 +1474,10 @@ public class Table return _overflow; } + public Object[] getRowValues() { + return _rowValues; + } + public void possiblyInvalidate(int modifiedPageNumber, ByteBuffer modifiedBuffer) { _rowBufferH.possiblyInvalidate(modifiedPageNumber, diff --git a/test/src/java/com/healthmarketscience/jackcess/DatabaseTest.java b/test/src/java/com/healthmarketscience/jackcess/DatabaseTest.java index 80c65cb..5235ef2 100644 --- a/test/src/java/com/healthmarketscience/jackcess/DatabaseTest.java +++ b/test/src/java/com/healthmarketscience/jackcess/DatabaseTest.java @@ -238,7 +238,8 @@ public class DatabaseTest extends TestCase { Object[] row3 = createTestRow("Tim3"); Table table = db.getTable("Test"); table.addRows(Arrays.asList(row1, row2, row3)); - + assertRowCount(3, table); + table.reset(); table.getNextRow(); table.getNextRow(); @@ -250,6 +251,7 @@ public class DatabaseTest extends TestCase { assertEquals("Tim1", outRow.get("A")); outRow = table.getNextRow(); assertEquals("Tim3", outRow.get("A")); + assertRowCount(2, table); // test multi row delete/add db = create(); @@ -261,29 +263,29 @@ public class DatabaseTest extends TestCase { table.addRow(row); } row[3] = 1974; - assertEquals(10, countRows(table)); + assertRowCount(10, table); table.reset(); table.getNextRow(); table.deleteCurrentRow(); - assertEquals(9, countRows(table)); + assertRowCount(9, table); table.reset(); table.getNextRow(); table.deleteCurrentRow(); - assertEquals(8, countRows(table)); + assertRowCount(8, table); table.reset(); for (int i = 0; i < 8; i++) { table.getNextRow(); } table.deleteCurrentRow(); - assertEquals(7, countRows(table)); + assertRowCount(7, table); table.addRow(row); - assertEquals(8, countRows(table)); + assertRowCount(8, table); table.reset(); for (int i = 0; i < 3; i++) { table.getNextRow(); } table.deleteCurrentRow(); - assertEquals(7, countRows(table)); + assertRowCount(7, table); table.reset(); assertEquals(2, table.getNextRow().get("D")); } @@ -607,7 +609,7 @@ public class DatabaseTest extends TestCase { new ArrayList<Object>(row.values())); table.reset(); - assertEquals(7, countRows(table)); + assertRowCount(7, table); } @@ -809,7 +811,14 @@ public class DatabaseTest extends TestCase { String str = builder.toString(); return str; } - + + static void assertRowCount(int expectedRowCount, Table table) + throws Exception + { + assertEquals(expectedRowCount, countRows(table)); + assertEquals(expectedRowCount, table.getRowCount()); + } + static int countRows(Table table) throws Exception { int rtn = 0; for(Map<String, Object> row : table) { diff --git a/test/src/java/com/healthmarketscience/jackcess/IndexTest.java b/test/src/java/com/healthmarketscience/jackcess/IndexTest.java index d8307a9..bbc0ab5 100644 --- a/test/src/java/com/healthmarketscience/jackcess/IndexTest.java +++ b/test/src/java/com/healthmarketscience/jackcess/IndexTest.java @@ -122,5 +122,45 @@ public class IndexTest extends TestCase { } } + public void testEntryDeletion() throws Exception { + File srcFile = new File("test/data/test.mdb"); + File dbFile = File.createTempFile("databaseTest", ".mdb"); + dbFile.deleteOnExit(); + copyFile(srcFile, dbFile); + + Table table = Database.open(dbFile).getTable("Table1"); + + for(int i = 0; i < 10; ++i) { + table.addRow("foo" + i, "bar" + i, (byte)42 + i, (short)53 + i, 13 * i, + (6.7d / i), null, null, true); + } + table.reset(); + assertRowCount(12, table); + + for(Index index : table.getIndexes()) { + assertEquals(12, index.getEntryCount()); + } + + table.reset(); + table.getNextRow(); + table.getNextRow(); + table.deleteCurrentRow(); + table.getNextRow(); + table.deleteCurrentRow(); + table.getNextRow(); + table.getNextRow(); + table.deleteCurrentRow(); + table.getNextRow(); + table.getNextRow(); + table.getNextRow(); + table.deleteCurrentRow(); + + table.reset(); + assertRowCount(8, table); + + for(Index index : table.getIndexes()) { + assertEquals(8, index.getEntryCount()); + } + } } |