From c0ba6a1b581d828f73cf841cc4b29eda0bc6f38b Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Sat, 24 Feb 2024 00:34:28 -0700 Subject: [PATCH] Fix bugs in moveColumns and renameColumns --- .../java/io/deephaven/engine/table/Table.java | 66 +++++- .../engine/table/impl/QueryTable.java | 170 +++++++-------- .../engine/table/impl/TableAdapter.java | 2 +- .../engine/table/impl/TableDefaults.java | 9 +- .../engine/table/impl/UncoalescedTable.java | 4 +- .../engine/table/impl/QueryTableTest.java | 193 +++++++++++++++++- .../engine/table/impl/TestMoveColumns.java | 48 +---- py/server/deephaven/table.py | 9 +- py/server/tests/test_update_graph.py | 15 -- 9 files changed, 351 insertions(+), 165 deletions(-) diff --git a/engine/api/src/main/java/io/deephaven/engine/table/Table.java b/engine/api/src/main/java/io/deephaven/engine/table/Table.java index 56091230495..32664ee234f 100644 --- a/engine/api/src/main/java/io/deephaven/engine/table/Table.java +++ b/engine/api/src/main/java/io/deephaven/engine/table/Table.java @@ -326,10 +326,49 @@ public interface Table extends @ConcurrentMethod Table dropColumnFormats(); + /** + * Produce a new table with the specified columns renamed using the syntax {@code "NewColumnName=OldColumnName"}. + *

+ * {@link IllegalArgumentException} will be thrown: + *

+ * + * @param pairs The columns to rename + * @return The new table, with the columns renamed + */ + @ConcurrentMethod Table renameColumns(Collection pairs); + /** + * Produce a new table with the specified columns renamed using the syntax {@code "NewColumnName=OldColumnName"}. + *

+ * {@link IllegalArgumentException} will be thrown: + *

+ * + * @param pairs The columns to rename + * @return The new table, with the columns renamed + */ + @ConcurrentMethod Table renameColumns(String... pairs); + /** + * Produce a new table with the specified columns renamed using the provided function. + *

+ * {@link IllegalArgumentException} will be thrown: + *

+ * + * @param renameFunction The function to apply to each column name + * @return The new table, with the columns renamed + */ + @ConcurrentMethod Table renameAllColumns(UnaryOperator renameFunction); @ConcurrentMethod @@ -342,8 +381,14 @@ public interface Table extends Table formatColumnWhere(String columnName, String condition, String formula); /** - * Produce a new table with the specified columns moved to the leftmost position. Columns can be renamed with the + * Produce a new table with the specified columns moved to the rightmost position. Columns can be renamed with the * usual syntax, i.e. {@code "NewColumnName=OldColumnName")}. + *

+ * {@link IllegalArgumentException} will be thrown: + *

    + *
  • if a source column is used more than once
    • + *
  • if a destination column is used more than once
    • + *
* * @param columnsToMove The columns to move to the left (and, optionally, to rename) * @return The new table, with the columns rearranged as explained above {@link #moveColumns(int, String...)} @@ -354,6 +399,12 @@ public interface Table extends /** * Produce a new table with the specified columns moved to the rightmost position. Columns can be renamed with the * usual syntax, i.e. {@code "NewColumnName=OldColumnName")}. + *

+ * {@link IllegalArgumentException} will be thrown: + *

    + *
  • if a source column is used more than once
    • + *
  • if a destination column is used more than once
    • + *
* * @param columnsToMove The columns to move to the right (and, optionally, to rename) * @return The new table, with the columns rearranged as explained above {@link #moveColumns(int, String...)} @@ -362,8 +413,14 @@ public interface Table extends Table moveColumnsDown(String... columnsToMove); /** - * Produce a new table with the specified columns moved to the specified {@code index}. Column indices begin at 0. - * Columns can be renamed with the usual syntax, i.e. {@code "NewColumnName=OldColumnName")}. + * Produce a new table with the specified columns moved to the rightmost position. Columns can be renamed with the + * usual syntax, i.e. {@code "NewColumnName=OldColumnName")}. + *

+ * {@link IllegalArgumentException} will be thrown: + *

    + *
  • if a source column is used more than once
    • + *
  • if a destination column is used more than once
    • + *
* * @param index The index to which the specified columns should be moved * @param columnsToMove The columns to move to the specified index (and, optionally, to rename) @@ -372,9 +429,6 @@ public interface Table extends @ConcurrentMethod Table moveColumns(int index, String... columnsToMove); - @ConcurrentMethod - Table moveColumns(int index, boolean moveToEnd, String... columnsToMove); - // ----------------------------------------------------------------------------------------------------------------- // Slice Operations // ----------------------------------------------------------------------------------------------------------------- diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java index 72c4d20aff0..45c71442f88 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java @@ -3,7 +3,6 @@ */ package io.deephaven.engine.table.impl; -import io.deephaven.UncheckedDeephavenException; import io.deephaven.api.AsOfJoinMatch; import io.deephaven.api.AsOfJoinRule; import io.deephaven.api.ColumnName; @@ -48,7 +47,6 @@ import io.deephaven.engine.table.impl.perf.QueryPerformanceNugget; import io.deephaven.engine.table.impl.rangejoin.RangeJoinOperation; import io.deephaven.engine.table.impl.select.MatchPairFactory; -import io.deephaven.engine.table.impl.select.SelectColumnFactory; import io.deephaven.engine.table.impl.updateby.UpdateBy; import io.deephaven.engine.table.impl.select.analyzers.SelectAndViewAnalyzerWrapper; import io.deephaven.engine.table.impl.sources.ring.RingTableTools; @@ -79,6 +77,7 @@ import io.deephaven.util.annotations.TestUseOnly; import io.deephaven.util.annotations.VisibleForTesting; import org.apache.commons.lang3.mutable.Mutable; +import org.apache.commons.lang3.mutable.MutableInt; import org.apache.commons.lang3.mutable.MutableObject; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -924,54 +923,11 @@ private String getCastFormulaInternal(Class dataType) { } @Override - public Table moveColumns(int index, boolean moveToEnd, String... columnsToMove) { + public Table moveColumns(final int index, String... columnsToMove) { final UpdateGraph updateGraph = getUpdateGraph(); try (final SafeCloseable ignored = ExecutionContext.getContext().withUpdateGraph(updateGraph).open()) { - // Get the current columns - final List currentColumns = getDefinition().getColumnNames(); - - // Create a Set from columnsToMove. This way, we can rename and rearrange columns at once. - final Set leftColsToMove = new HashSet<>(); - final Set rightColsToMove = new HashSet<>(); - int extraCols = 0; - - for (final String columnToMove : columnsToMove) { - final String left = MatchPairFactory.getExpression(columnToMove).leftColumn; - final String right = MatchPairFactory.getExpression(columnToMove).rightColumn; - - if (!leftColsToMove.add(left) || !currentColumns.contains(left) || (rightColsToMove.contains(left) - && !left.equals(right) && leftColsToMove.stream().anyMatch(col -> col.equals(right)))) { - extraCols++; - } - if (currentColumns.stream().anyMatch(currentColumn -> currentColumn.equals(right)) - && !left.equals(right) - && rightColsToMove.add(right) && !rightColsToMove.contains(left)) { - extraCols--; - } - } - index += moveToEnd ? extraCols : 0; - - // vci for write, cci for currentColumns, ctmi for columnsToMove - final SelectColumn[] viewColumns = new SelectColumn[currentColumns.size() + extraCols]; - for (int vci = 0, cci = 0, ctmi = 0; vci < viewColumns.length;) { - if (vci >= index && ctmi < columnsToMove.length) { - viewColumns[vci++] = SelectColumnFactory.getExpression(columnsToMove[ctmi++]); - } else { - // Don't add the column if it's one of the columns we're moving or if it has been renamed. - final String currentColumn = currentColumns.get(cci++); - if (!leftColsToMove.contains(currentColumn) - && Arrays.stream(viewColumns).noneMatch( - viewCol -> viewCol != null - && viewCol.getMatchPair().leftColumn.equals(currentColumn)) - && Arrays.stream(columnsToMove) - .noneMatch(colToMove -> MatchPairFactory.getExpression(colToMove).rightColumn - .equals(currentColumn))) { - - viewColumns[vci++] = SelectColumnFactory.getExpression(currentColumn); - } - } - } - return viewOrUpdateView(Flavor.View, viewColumns); + final MatchPair[] pairsToMove = MatchPairFactory.getExpressions(columnsToMove); + return renameColumnsImpl("moveColumns(" + index + ", ", Math.max(0, index), pairsToMove); } } @@ -1820,73 +1776,121 @@ public void onUpdate(final TableUpdate upstream) { public Table renameColumns(Collection pairs) { final UpdateGraph updateGraph = getUpdateGraph(); try (final SafeCloseable ignored = ExecutionContext.getContext().withUpdateGraph(updateGraph).open()) { - return renameColumnsImpl(MatchPair.fromPairs(pairs)); + return renameColumnsImpl("renameColumns(", -1, MatchPair.fromPairs(pairs)); } } - private Table renameColumnsImpl(MatchPair... pairs) { - return QueryPerformanceRecorder.withNugget("renameColumns(" + matchString(pairs) + ")", + private Table renameColumnsImpl( + @NotNull final String methodNuggetPrefix, + final int movePosition, + @NotNull final MatchPair... pairs) { + return QueryPerformanceRecorder.withNugget(methodNuggetPrefix + matchString(pairs) + ")", sizeForInstrumentation(), () -> { if (pairs == null || pairs.length == 0) { return prepareReturnThis(); } - checkInitiateOperation(); - - final Map pairLookup = new HashMap<>(); + final Set newNames = new HashSet<>(); + final Map pairLookup = new LinkedHashMap<>(); for (final MatchPair pair : pairs) { - if (pair.leftColumn == null || pair.leftColumn.equals("")) { + if (pair.leftColumn == null || pair.leftColumn.isEmpty()) { throw new IllegalArgumentException( "Bad left column in rename pair \"" + pair + "\""); } if (null == columns.get(pair.rightColumn)) { throw new IllegalArgumentException("Column \"" + pair.rightColumn + "\" not found"); } - pairLookup.put(pair.rightColumn, pair.leftColumn); + if (pairLookup.put(pair.rightColumn, pair.leftColumn) != null) { + throw new IllegalArgumentException( + "Duplicate source column \"" + pair.rightColumn + "\""); + } + if (!newNames.add(pair.leftColumn)) { + throw new IllegalArgumentException( + "Duplicate destination column \"" + pair.leftColumn + "\""); + } } - int mcsPairIdx = 0; + final MutableInt mcsPairIdx = new MutableInt(); final MatchPair[] modifiedColumnSetPairs = new MatchPair[columns.size()]; - final Map> newColumns = new LinkedHashMap<>(); + + final Runnable moveColumns = () -> { + for (final Map.Entry rename : pairLookup.entrySet()) { + final String oldName = rename.getKey(); + final String newName = rename.getValue(); + final ColumnSource columnSource = columns.get(oldName); + newColumns.put(newName, columnSource); + modifiedColumnSetPairs[mcsPairIdx.getAndIncrement()] = new MatchPair(newName, oldName); + } + }; + for (final Map.Entry> entry : columns.entrySet()) { final String oldName = entry.getKey(); final ColumnSource columnSource = entry.getValue(); String newName = pairLookup.get(oldName); if (newName == null) { + if (newNames.contains(oldName)) { + // this column is being replaced by a rename + continue; + } newName = oldName; + } else if (movePosition >= 0) { + // we move this column when we get to the right position + continue; + } + + if (mcsPairIdx.intValue() == movePosition) { + moveColumns.run(); } - modifiedColumnSetPairs[mcsPairIdx++] = new MatchPair(newName, oldName); + + modifiedColumnSetPairs[mcsPairIdx.getAndIncrement()] = new MatchPair(newName, oldName); newColumns.put(newName, columnSource); } - final QueryTable queryTable = new QueryTable(rowSet, newColumns); - if (isRefreshing()) { - final ModifiedColumnSet.Transformer mcsTransformer = - newModifiedColumnSetTransformer(queryTable, modifiedColumnSetPairs); - addUpdateListener(new ListenerImpl("renameColumns(" + Arrays.deepToString(pairs) + ')', - this, queryTable) { - @Override - public void onUpdate(final TableUpdate upstream) { - final TableUpdateImpl downstream = TableUpdateImpl.copy(upstream); - downstream.modifiedColumnSet = queryTable.getModifiedColumnSetForUpdates(); - if (upstream.modified().isNonempty()) { - mcsTransformer.clearAndTransform(upstream.modifiedColumnSet(), - downstream.modifiedColumnSet); - } else { - downstream.modifiedColumnSet.clear(); - } - queryTable.notifyListeners(downstream); - } - }); + if (mcsPairIdx.intValue() <= movePosition) { + moveColumns.run(); } - propagateFlatness(queryTable); - copyAttributes(queryTable, CopyAttributeOperation.RenameColumns); - copySortableColumns(queryTable, pairs); - maybeCopyColumnDescriptions(queryTable, pairs); + final Mutable result = new MutableObject<>(); + + final OperationSnapshotControl snapshotControl = + createSnapshotControlIfRefreshing(OperationSnapshotControl::new); + initializeWithSnapshot("renameColumns", snapshotControl, (usePrev, beforeClockValue) -> { + final QueryTable resultTable = new QueryTable(rowSet, newColumns); + propagateFlatness(resultTable); + + copyAttributes(resultTable, CopyAttributeOperation.RenameColumns); + copySortableColumns(resultTable, pairs); + maybeCopyColumnDescriptions(resultTable, pairs); + + if (snapshotControl != null) { + final ModifiedColumnSet.Transformer mcsTransformer = + newModifiedColumnSetTransformer(resultTable, modifiedColumnSetPairs); + final ListenerImpl listener = new ListenerImpl( + methodNuggetPrefix + Arrays.deepToString(pairs) + ')', this, resultTable) { + @Override + public void onUpdate(final TableUpdate upstream) { + final TableUpdateImpl downstream = TableUpdateImpl.copy(upstream); + downstream.modifiedColumnSet = resultTable.getModifiedColumnSetForUpdates(); + if (upstream.modified().isNonempty()) { + mcsTransformer.clearAndTransform(upstream.modifiedColumnSet(), + downstream.modifiedColumnSet); + } else { + downstream.modifiedColumnSet.clear(); + } + resultTable.notifyListeners(downstream); + } + }; + snapshotControl.setListenerAndResult(listener, resultTable); + } + + result.setValue(resultTable); + + return true; + }); + + return result.getValue(); - return queryTable; }); } diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/TableAdapter.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/TableAdapter.java index 4b25f59be2a..c724ad397bb 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/TableAdapter.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/TableAdapter.java @@ -219,7 +219,7 @@ default Table renameColumns(Collection pairs) { } @Override - default Table moveColumns(int index, boolean moveToEnd, String... columnsToMove) { + default Table moveColumns(int index, String... columnsToMove) { return throwUnsupported(); } diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/TableDefaults.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/TableDefaults.java index 836ca71260e..f4e251f227f 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/TableDefaults.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/TableDefaults.java @@ -243,14 +243,7 @@ default Table moveColumnsUp(String... columnsToMove) { @ConcurrentMethod @FinalDefault default Table moveColumnsDown(String... columnsToMove) { - return moveColumns(numColumns() - columnsToMove.length, true, columnsToMove); - } - - @Override - @ConcurrentMethod - @FinalDefault - default Table moveColumns(int index, String... columnsToMove) { - return moveColumns(index, false, columnsToMove); + return moveColumns(numColumns() - columnsToMove.length, columnsToMove); } // ----------------------------------------------------------------------------------------------------------------- diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/UncoalescedTable.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/UncoalescedTable.java index 4599d3cdd4b..23c827532e1 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/UncoalescedTable.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/UncoalescedTable.java @@ -271,8 +271,8 @@ public Table renameColumns(Collection pairs) { @Override @ConcurrentMethod - public Table moveColumns(int index, boolean moveToEnd, String... columnsToMove) { - return coalesce().moveColumns(index, moveToEnd, columnsToMove); + public Table moveColumns(int index, String... columnsToMove) { + return coalesce().moveColumns(index, columnsToMove); } @Override diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java index 58ef8ac5996..4d8b0708dec 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableTest.java @@ -664,6 +664,25 @@ public void testRenameColumns() { fail("Expected exception"); } catch (RuntimeException ignored) { } + + // Can't rename to a dest column twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.renameColumns("O = Int", "O = Int"); + }); + + // Check what happens when we override a column by name + final Table override = table.renameColumns("Double = Int"); + Assert.assertEquals(override.getColumnSource("Double").getType(), int.class); + Assert.assertFalse(override.getColumnSourceMap().containsKey("Int")); + // Check that ordering of source columns does not matter + final Table override2 = table.renameColumns("Int = Double"); + Assert.assertEquals(override2.getColumnSource("Int").getType(), double.class); + Assert.assertFalse(override2.getColumnSourceMap().containsKey("Double")); + + // Validate that we can swap two columns simultaneously + final Table swapped = table.renameColumns("Double = Int", "Int = Double"); + Assert.assertEquals(swapped.getColumnSource("Double").getType(), int.class); + Assert.assertEquals(swapped.getColumnSource("Int").getType(), double.class); } public void testRenameColumnsIncremental() { @@ -680,14 +699,9 @@ public void testRenameColumnsIncremental() { final EvalNugget[] en = new EvalNugget[] { EvalNugget.from(() -> queryTable.renameColumns(List.of())), EvalNugget.from(() -> queryTable.renameColumns("Symbol=Sym")), - EvalNugget.from(() -> queryTable.renameColumns("Symbol=Sym", "Symbols=Sym")), EvalNugget.from(() -> queryTable.renameColumns("Sym2=Sym", "intCol2=intCol", "doubleCol2=doubleCol")), }; - // Verify our assumption that columns can be renamed at most once. - Assert.assertTrue(queryTable.renameColumns("Symbol=Sym", "Symbols=Sym").hasColumns("Symbols")); - Assert.assertFalse(queryTable.renameColumns("Symbol=Sym", "Symbols=Sym").hasColumns("Symbol")); - final int steps = 100; for (int i = 0; i < steps; i++) { if (printTableUpdates) { @@ -697,6 +711,175 @@ public void testRenameColumnsIncremental() { } } + public void testMoveColumnsUp() { + final Table table = emptyTable(1).update("A = 1", "B = 2", "C = 3", "D = 4", "E = 5"); + + assertTableEquals( + emptyTable(1).update("C = 3", "A = 1", "B = 2", "D = 4", "E = 5"), + table.moveColumnsUp("C")); + + assertTableEquals( + emptyTable(1).update("C = 3", "B = 2", "A = 1", "D = 4", "E = 5"), + table.moveColumnsUp("C", "B")); + + assertTableEquals( + emptyTable(1).update("D = 4", "A = 1", "C = 3", "B = 2", "E = 5"), + table.moveColumnsUp("D", "A", "C")); + + // test trivial do nothing case + assertTableEquals(table, table.moveColumnsUp()); + + // Can't move a column up twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumnsUp("C", "C"); + }); + + // Can't rename a source column twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumnsUp("A1 = A", "A2 = A"); + }); + + // Can't rename to a dest column twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumnsUp("O = A", "O = B"); + }); + + assertTableEquals( + emptyTable(1).update("B = 3", "A = 1", "D = 4", "E = 5"), + table.moveColumnsUp("B = C")); + assertTableEquals( + emptyTable(1).update("B = 1", "C = 3", "D = 4", "E = 5"), + table.moveColumnsUp("B = A")); + assertTableEquals( + emptyTable(1).update("B = 1", "A = 2", "C = 3", "D = 4", "E = 5"), + table.moveColumnsUp("B = A", "A = B")); + } + + public void testMoveColumnsDown() { + final Table table = emptyTable(1).update("A = 1", "B = 2", "C = 3", "D = 4", "E = 5"); + + assertTableEquals( + emptyTable(1).update("A = 1", "B = 2", "D = 4", "E = 5", "C = 3"), + table.moveColumnsDown("C")); + + assertTableEquals( + emptyTable(1).update("A = 1", "D = 4", "E = 5", "C = 3", "B = 2"), + table.moveColumnsDown("C", "B")); + + assertTableEquals( + emptyTable(1).update("B = 2", "E = 5", "D = 4", "A = 1", "C = 3"), + table.moveColumnsDown("D", "A", "C")); + + // test trivial do nothing case + assertTableEquals(table, table.moveColumnsDown()); + + // Can't move a column down twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumnsDown("C", "C"); + }); + + // Can't rename a source column twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumnsDown("A1 = A", "A2 = A"); + }); + + // Can't rename to a dest column twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumnsDown("O = A", "O = B"); + }); + + assertTableEquals( + emptyTable(1).update("A = 1", "D = 4", "E = 5", "B = 3"), + table.moveColumnsDown("B = C")); + assertTableEquals( + emptyTable(1).update("C = 3", "D = 4", "E = 5", "B = 1"), + table.moveColumnsDown("B = A")); + assertTableEquals( + emptyTable(1).update("C = 3", "D = 4", "E = 5", "B = 1", "A = 2"), + table.moveColumnsDown("B = A", "A = B")); + } + + public void testMoveColumns() { + final Table table = emptyTable(1).update("A = 1", "B = 2", "C = 3", "D = 4", "E = 5"); + + // single column + assertTableEquals( + emptyTable(1).update("C = 3", "A = 1", "B = 2", "D = 4", "E = 5"), + table.moveColumns(-1, "C")); + assertTableEquals( + emptyTable(1).update("C = 3", "A = 1", "B = 2", "D = 4", "E = 5"), + table.moveColumns(0, "C")); + assertTableEquals( + emptyTable(1).update("A = 1", "C = 3", "B = 2", "D = 4", "E = 5"), + table.moveColumns(1, "C")); + assertTableEquals( + emptyTable(1).update("A = 1", "B = 2", "C = 3", "D = 4", "E = 5"), + table.moveColumns(2, "C")); + assertTableEquals( + emptyTable(1).update("A = 1", "B = 2", "D = 4", "C = 3", "E = 5"), + table.moveColumns(3, "C")); + assertTableEquals( + emptyTable(1).update("A = 1", "B = 2", "D = 4", "E = 5", "C = 3"), + table.moveColumns(4, "C")); + assertTableEquals( + emptyTable(1).update("A = 1", "B = 2", "D = 4", "E = 5", "C = 3"), + table.moveColumns(10, "C")); + + // two columns + assertTableEquals( + emptyTable(1).update("C = 3", "B = 2", "A = 1", "D = 4", "E = 5"), + table.moveColumns(-1, "C", "B")); + assertTableEquals( + emptyTable(1).update("C = 3", "B = 2", "A = 1", "D = 4", "E = 5"), + table.moveColumns(0, "C", "B")); + assertTableEquals( + emptyTable(1).update("A = 1", "C = 3", "B = 2", "D = 4", "E = 5"), + table.moveColumns(1, "C", "B")); + assertTableEquals( + emptyTable(1).update("A = 1", "D = 4", "C = 3", "B = 2", "E = 5"), + table.moveColumns(2, "C", "B")); + assertTableEquals( + emptyTable(1).update("A = 1", "D = 4", "E = 5", "C = 3", "B = 2"), + table.moveColumns(3, "C", "B")); + assertTableEquals( + emptyTable(1).update("A = 1", "D = 4", "E = 5", "C = 3", "B = 2"), + table.moveColumns(4, "C", "B")); + assertTableEquals( + emptyTable(1).update("A = 1", "D = 4", "E = 5", "C = 3", "B = 2"), + table.moveColumns(10, "C", "B")); + + // test trivial do nothing case + for (int ii = -1; ii < 10; ++ii) { + assertTableEquals(table, table.moveColumns(ii)); + } + + // Can't move a column down twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumns(2, "C", "C"); + }); + + // Can't rename a source column twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumns(2, "A1 = A", "A2 = A"); + }); + + // Can't rename to a dest column twice. + Assert.assertThrows(IllegalArgumentException.class, () -> { + table.moveColumns(2, "O = A", "O = B"); + }); + + + assertTableEquals( + emptyTable(1).update("A = 1", "D = 4", "B = 3", "E = 5"), + table.moveColumns(2, "B = C")); + assertTableEquals( + emptyTable(1).update("C = 3", "D = 4", "B = 1", "E = 5"), + table.moveColumns(2, "B = A")); + assertTableEquals( + emptyTable(1).update("C = 3", "D = 4", "B = 1", "A = 2", "E = 5"), + table.moveColumns(2, "B = A", "A = B")); + } + public static WhereFilter stringContainsFilter( String columnName, String... values) { diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/TestMoveColumns.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/TestMoveColumns.java index 3edb395a20b..f82f26c7978 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/TestMoveColumns.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/TestMoveColumns.java @@ -69,29 +69,13 @@ public void testMoveColumns() { checkColumnOrder(temp, "dexab"); checkColumnValueOrder(temp, "45123"); - temp = table.moveColumns(0, "x=a", "b=x"); - checkColumnOrder(temp, "xbcde"); - checkColumnValueOrder(temp, "11345"); - - temp = table.moveColumns(0, "x=a", "y=a", "z=a"); - checkColumnOrder(temp, "xyzbcde"); - checkColumnValueOrder(temp, "1112345"); + temp = table.moveColumns(0, "x=a", "d"); + checkColumnOrder(temp, "xdbce"); + checkColumnValueOrder(temp, "14235"); temp = table.moveColumns(0, "b=a", "a=b"); checkColumnOrder(temp, "bacde"); - checkColumnValueOrder(temp, "11345"); - - temp = table.moveColumns(0, "d=c", "d=a", "x=e"); - checkColumnOrder(temp, "dxb"); - checkColumnValueOrder(temp, "152"); - - temp = table.moveColumns(0, "a=b", "a=c"); - checkColumnOrder(temp, "ade"); - checkColumnValueOrder(temp, "345"); - - temp = table.moveColumns(0, "a=b", "a=c", "a=d", "a=e"); - checkColumnOrder(temp, "a"); - checkColumnValueOrder(temp, "5"); + checkColumnValueOrder(temp, "12345"); } public void testMoveUpColumns() { @@ -110,14 +94,6 @@ public void testMoveUpColumns() { temp = table.moveColumnsUp("x=e"); checkColumnOrder(temp, "xabcd"); checkColumnValueOrder(temp, "51234"); - - temp = table.moveColumnsUp("x=a", "x=b"); - checkColumnOrder(temp, "xcde"); - checkColumnValueOrder(temp, "2345"); - - temp = table.moveColumnsUp("x=a", "y=a"); - checkColumnOrder(temp, "xybcde"); - checkColumnValueOrder(temp, "112345"); } public void testMoveDownColumns() { @@ -134,19 +110,11 @@ public void testMoveDownColumns() { temp = table.moveColumnsDown("b=a", "a=b", "c"); checkColumnOrder(temp, "debac"); - checkColumnValueOrder(temp, "45113"); - - temp = table.moveColumnsDown("b=a", "a=b", "c", "d=a"); - checkColumnOrder(temp, "ebacd"); - checkColumnValueOrder(temp, "51131"); - - temp = table.moveColumnsDown("x=a", "x=b"); - checkColumnOrder(temp, "cdex"); - checkColumnValueOrder(temp, "3452"); + checkColumnValueOrder(temp, "45123"); - temp = table.moveColumnsDown("x=a", "y=a"); - checkColumnOrder(temp, "bcdexy"); - checkColumnValueOrder(temp, "234511"); + temp = table.moveColumnsDown("b=a", "a=b", "c"); + checkColumnOrder(temp, "debac"); + checkColumnValueOrder(temp, "45123"); } private void checkColumnOrder(Table t, String expectedOrder) { diff --git a/py/server/deephaven/table.py b/py/server/deephaven/table.py index 922e6b3dcd1..d09c34d865f 100644 --- a/py/server/deephaven/table.py +++ b/py/server/deephaven/table.py @@ -80,11 +80,11 @@ class NodeType(Enum): """An enum of node types for RollupTable""" AGGREGATED = _JNodeType.Aggregated """Nodes at an aggregated (rolled up) level in the RollupTable. An aggregated level is above the constituent ( - leaf) level. These nodes have column names and types that result from applying aggregations on the source table + leaf) level. These nodes have column names and types that result from applying aggregations on the source table of the RollupTable. """ CONSTITUENT = _JNodeType.Constituent - """Nodes at the leaf level when :meth:`~deephaven.table.Table.rollup` method is called with - include_constituent=True. The constituent level is the lowest in a rollup table. These nodes have column names + """Nodes at the leaf level when :meth:`~deephaven.table.Table.rollup` method is called with + include_constituent=True. The constituent level is the lowest in a rollup table. These nodes have column names and types from the source table of the RollupTable. """ @@ -734,8 +734,7 @@ def rename_columns(self, cols: Union[str, Sequence[str]]) -> Table: """ try: cols = to_sequence(cols) - with auto_locking_ctx(self): - return Table(j_table=self.j_table.renameColumns(*cols)) + return Table(j_table=self.j_table.renameColumns(*cols)) except Exception as e: raise DHError(e, "table rename_columns operation failed.") from e diff --git a/py/server/tests/test_update_graph.py b/py/server/tests/test_update_graph.py index 2cdf0d116cc..4da6db8e058 100644 --- a/py/server/tests/test_update_graph.py +++ b/py/server/tests/test_update_graph.py @@ -161,21 +161,6 @@ def test_auto_locking_joins(self): with self.subTest(op=op): result_table = left_table.aj(right_table, on="X") - def test_auto_locking_rename_columns(self): - with ug.shared_lock(self.test_update_graph): - test_table = time_table("PT00:00:00.001").update(["X=i", "Y=i%13", "Z=X*Y"]) - - cols_to_rename = [ - f"{f.name + '_2'} = {f.name}" for f in test_table.columns[::2] - ] - - with self.assertRaises(DHError) as cm: - result_table = test_table.rename_columns(cols_to_rename) - self.assertRegex(str(cm.exception), r"IllegalStateException") - - ug.auto_locking = True - result_table = test_table.rename_columns(cols_to_rename) - def test_auto_locking_ungroup(self): with ug.shared_lock(self.test_update_graph): test_table = time_table("PT00:00:00.001").update(["X=i", "Y=i%13"])