Add TableDefinition column name helpers #4813
Conversation
This is a potentially useful built-in, along with TableDefinition#getColumnNameSet. As part of this change, NoSuchColumnException was moved to table-api. The win here is that BaseTable#checkAvailableColumns no longer coalesces, and BaseTable and HierarchicalTableImpls are straightforward without duplicated logic.
devinrsmith
added
clean up
NoDocumentationNeeded
NoReleaseNotesNeeded
engine/api/src/main/java/io/deephaven/engine/table/TableDefinition.java
Outdated
Show resolved
Hide resolved
engine/api/src/main/java/io/deephaven/engine/table/TableDefinition.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/CrossJoinHelper.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/test/java/io/deephaven/engine/table/impl/SelectOverheadLimiter.java
Outdated
Show resolved
Hide resolved
engine/table/src/test/java/io/deephaven/engine/table/impl/SelectOverheadLimiter.java
Outdated
Show resolved
Hide resolved
| @@ -818,7 +818,7 @@ public void testAggAllByWithFormatColumn() { | |||
| assertEquals(2.0, cs.get(0)); | |||
|
|
|||
| result = dataTable.formatColumns("Doubles=Decimal(`##0.00%`)").headBy(1); | |||
| Set<String> columnNames = result.getColumnSourceMap().keySet(); | |||
| List<String> columnNames = result.getDefinition().getColumnNames(); | |||
There was a problem hiding this comment.
Why not use your new set method?
There was a problem hiding this comment.
test impl only checks size and iterates through it - no need for Set afaict.
There was a problem hiding this comment.
Sure. Just a question of an on-demand allocation vs. a cached one.
There was a problem hiding this comment.
Ah, I see the "concern"; this brings up the larger question of do we want / need to document the APIs in terms of whether they are cached or not / implementation details? I don't necessarily want to box us into defining TableDefinition#getColumnNames() as on-demand, TableDefinition#getColumnNamesSet as cached.
There was a problem hiding this comment.
It's not unreasonable IMO that we could make the case that TableDefinition impl should go from
private final List<ColumnDefinition<?>> columns;
private Map<String, ColumnDefinition<?>> columnNameMap;to
private final Map<String, ColumnDefinition<?>> columnNameMap;. Majority of work is probably auditing usage of io.deephaven.engine.table.TableDefinition#getColumns() to see who actually needs a List vs a Collection (which could be achieved efficiently by columnNameMap.values()).
There was a problem hiding this comment.
I suspect you're right that the vast majority of the time an ordered collection is sufficient. No need to scope creep this PR for that.
...nsions/parquet/table/src/test/java/io/deephaven/parquet/table/ParquetTableReadWriteTest.java
Outdated
Show resolved
Hide resolved
...nsions/parquet/table/src/test/java/io/deephaven/parquet/table/ParquetTableReadWriteTest.java
Outdated
Show resolved
Hide resolved
engine/api/src/main/java/io/deephaven/engine/table/impl/NoSuchColumnException.java
Show resolved
Hide resolved
.../table/src/main/java/io/deephaven/engine/table/impl/by/ChunkedOperatorAggregationHelper.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateBy.java
Outdated
Show resolved
Hide resolved
engine/test-utils/src/main/java/io/deephaven/engine/testutil/TstUtils.java
Outdated
Show resolved
Hide resolved
engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableAggregationTest.java
Show resolved
Hide resolved
Adds some helper methods for on
TableDefinition#checkHasColumn,TableDefinition#checkHasColumns, andTableDefinition#getColumnNameSet.Additionally, fixes up call sites that were (ab)using
Table#getColumnSourceMapto simply get the keySet. This invokes a potentially extraneousTable#coalescewhich can be avoided in these cases.In support of common scaffolding so #4771 won't need to call
Table#getColumnSourcefor validation purposes.