Table operations #762

devinrsmith · 2021-06-17T19:06:30Z

A non-comprehensive first pass at extracting table operations api from Table.

First commit is table-api module standalone.

Second commit is incorporating table-api into DB/Table.

devinrsmith · 2021-06-17T19:08:21Z

Documentation requirements: targeted javadocs.

devinrsmith · 2021-06-23T15:54:57Z

Part of this PR (which way may need to revisit) is appending 2 to some new table operation methods, due to non-overloadability of java generics. For example:

    TOPS updateView(Collection<String> columns);
    TOPS updateView2(Collection<Selectable> columns);

Looks like this is causing some sort of auto-completion failure.

So, should we:

Fix the autocomplete tests and live w/ the 2 methods?
Choose a different prefix for these new semi-overloads? newUpdateView(Collection<Selectable> columns)
Something else?

Error:

 ColumnExpressionCompletionHandlerTest.Completion should offer column name completion from when a where clause contains an unclosed String with a partial prefix (cursor position 19)

Condition not satisfied:

result.size() == 2
|      |      |
|      3      false
[CompletionItem{start=7, length=12, textEdit=TextEdit{range=Range{start=Position{line=1, character=6}, end=Position{line=1, character=18}}, text='updateView2('}}
, CompletionItem{start=20, length=3, textEdit=TextEdit{range=Range{start=Position{line=1, character=19}, end=Position{line=1, character=22}}, text=''Date = 
'}}
, CompletionItem{start=20, length=3, textEdit=TextEdit{range=Range{start=Position{line=1, character=19}, end=Position{line=1, character=22}}, text=''Delta = 
'}}
]

JamesXNelson · 2021-06-23T17:21:41Z

updateView2 is a terrible pattern. Naming is hard, but we should come up with something more descriptive.

If the new name does start w/ updateView, it's fine to just add more cases to the test.

devinrsmith · 2021-06-23T19:00:07Z

Current plan is to delete Collection<String> methods, and use Collection<StrongType>, so we can use the same overloaded name.

…ctions of strings

table-api/src/main/java/io/deephaven/api/TableOperations.java

table-api/src/main/java/io/deephaven/api/agg/Aggregation.java

table-api/src/main/java/io/deephaven/api/TableOperations.java

table-api/src/main/java/io/deephaven/api/ColumnMatch.java

table-api/src/main/java/io/deephaven/api/ColumnName.java

table-api/src/main/java/io/deephaven/api/ColumnAssignment.java

DB/src/main/java/io/deephaven/db/v2/by/ComboAggregateFactory.java

JamesXNelson

Left quite a few comments regarding docs. Given the size of this PR and the urgency, I think it'd be okay if we just translate these requests into a "Document Table Operations API" issue.

I would like to see the vanila error messages approved before merging though.

JamesXNelson · 2021-06-30T17:45:54Z

DB/src/main/java/io/deephaven/db/tables/Table.java

-    /**
-     * Perform a cross join with the right table.
-     * <p>
-     * Returns a table that is the cartesian product of left rows X right rows, with one column for each of the left
-     * table's columns, and one column corresponding to each of the right table's columns that are included in the
-     * columnsToAdd argument. The rows are ordered first by the left table then by the right table. If columnsToMatch
-     * is non-empty then the product is filtered by the supplied match conditions.
-     * <p>
-     * To efficiently produce updates, the bits that represent a key for a given row are split into two. Unless specified,
-     * join reserves 16 bits to represent a right row. When there are too few bits to represent all of the right rows
-     * for a given aggregation group the table will shift a bit from the left side to the right side. The default of 16
-     * bits was carefully chosen because it results in an efficient implementation to process live updates.
-     * <p>
-     * An {@link io.deephaven.db.v2.utils.OutOfKeySpaceException} is thrown when the total number of bits needed
-     * to express the result table exceeds that needed to represent Long.MAX_VALUE. There are a few work arounds:
-     * - If the left table is sparse, consider flattening the left table.
-     * - If there are no key-columns and the right table is sparse, consider flattening the right table.
-     * - If the maximum size of a right table's group is small, you can reserve fewer bits by setting numRightBitsToReserve on initialization.
-     * <p>
-     * Note: If you can prove that a given group has at most one right-row then you should prefer using {@link #naturalJoin}.
-     *
-     * @param rightTable     The right side table on the join.
-     * @param columnsToMatch A collection of match conditions ("leftColumn=rightColumn" or "columnFoundInBoth")
-     * @param columnsToAdd   A collection of the columns from the right side that need to be added to the left
-     *                       side as a result of the match.
-     * @return a new table joined according to the specification in columnsToMatch and columnsToAdd
-     */


Has this javadoc moved somewhere? I don't see it offhand, but maybe hidden under a [+] somewhere...

JamesXNelson · 2021-06-30T17:47:27Z

DB/src/main/java/io/deephaven/db/tables/select/MatchPair.java

+
+        @Override
+        public void visit(ColumnName columnName) {
+            out = new MatchPair(columnName.name(), columnName.name());


does it make sense to validate that out == null before assignment?

Technically, yes - the visitor pattern where I'm expecting a single call and single output it is a bit more correct to ensure it hasn't been invoked yet. I usually handle the output side checking that it's not null on return. That said, it's internal code, so it's not something an end-user can invoke incorrectly. In my latest code, this specific visitor no longer exists.

table-api/src/main/java/io/deephaven/api/AsOfJoinRule.java

table-api/src/main/java/io/deephaven/api/ColumnAssignment.java

table-api/src/main/java/io/deephaven/api/ColumnFormula.java

table-api/src/main/java/io/deephaven/db/tables/utils/DBNameValidator.java

JamesXNelson · 2021-06-30T18:27:05Z

table-api/src/test/java/io/deephaven/api/ColumnAssignmentTest.java

+    private void expectParseFailure(String x) {
+        try {
+            ColumnAssignment.parse(x);
+            failBecauseExceptionWasNotThrown(IllegalArgumentException.class);
+        } catch (IllegalArgumentException e) {
+            // ignore
+        }
+    }


It's perfectly legal to do:

@Test(expected=IllegalArgumentException.class) void doSomething() { ColumnAssignment.parse("Foo≡Bar") }

This annotation doesn't work in Junit 5 (what I'm using here). There may be a more canonical way to express expectations for failures in Junit 5?

table-api/src/main/java/io/deephaven/db/tables/utils/DBNameValidator.java

table-api/src/main/java/io/deephaven/api/agg/WAvg.java

table-api/src/main/java/io/deephaven/api/TableOperations.java

table-api/src/main/java/io/deephaven/api/agg/Aggregation.java

table-api/src/main/java/io/deephaven/api/agg/SortedFirst.java

table-api/src/main/java/io/deephaven/api/Expression.java

DB/src/main/java/io/deephaven/db/v2/by/ComboAggregateFactory.java

table-api/src/main/java/io/deephaven/api/agg/SortedFirst.java

DB/src/main/java/io/deephaven/db/v2/select/SelectFilter.java

settings.gradle

DB/src/main/java/io/deephaven/db/v2/by/ComboAggregateFactory.java

DB/src/main/java/io/deephaven/db/v2/select/SelectColumn.java

table-api/src/main/java/io/deephaven/api/JoinAdditionImpl.java

DB/src/main/java/io/deephaven/db/v2/select/SelectFilter.java

table-api/src/main/java/io/deephaven/api/filter/FilterCondition.java

DB/src/main/java/io/deephaven/db/v2/select/SelectFilter.java

DB/src/main/java/io/deephaven/db/v2/by/ComboAggregateFactory.java

table-api/src/main/java/io/deephaven/api/agg/AggregationOutputs.java

devinrsmith added 2 commits June 17, 2021 12:02

Initial table-api with TableOperations

de5f714

Incorporate table-api into DB

984befe

devinrsmith added table api core Core development tasks labels Jun 17, 2021

devinrsmith added this to the June 2021 milestone Jun 17, 2021

devinrsmith requested a review from rcaudy June 17, 2021 19:06

devinrsmith self-assigned this Jun 17, 2021

devinrsmith added the DocumentationNeeded label Jun 17, 2021

rcaudy self-assigned this Jun 18, 2021

devinrsmith requested a review from JamesXNelson June 23, 2021 15:49

Update TableOperations with same overloaded methods by removing colle…

cb43d5b

…ctions of strings

rcaudy reviewed Jun 25, 2021

View reviewed changes

devinrsmith added 3 commits June 28, 2021 12:16

Take latest changes from java-client

9cb3ecd

Review responses

299b283

Merge remote-tracking branch 'upstream/main' into table-operations

76bbd70

devinrsmith requested a review from rcaudy June 28, 2021 22:07

JamesXNelson reviewed Jun 30, 2021

View reviewed changes

rcaudy reviewed Jul 2, 2021

View reviewed changes

devinrsmith mentioned this pull request Jul 6, 2021

Allow more table names #822

Open

devinrsmith added 7 commits July 7, 2021 12:04

Merge remote-tracking branch 'upstream/main' into table-operations

e1aebaa

Brought in latest from java-client-personal

2ed132e

filter, value, expression stuff

e9070bb

Fix compile error

458e564

Make ColumnName implement Selectable

d12d72a

Make ColumnName implement Pair

da0ac25

Fix compile error

85d6a69

devinrsmith requested a review from rcaudy July 8, 2021 18:05

devinrsmith added 6 commits July 8, 2021 11:28

Make ColumnName implement JoinMatch

a67596b

Make ColumnName implement JoinAddition

35c2c45

SortColumn helpers from ColumnName

10ee521

AggregationFinisher works on Pair now

55904cc

PairImpl / SelectableImpl serializable

65f9195

Add join without reserve bits

649d03a

rcaudy reviewed Jul 14, 2021

View reviewed changes

Review responses

02004f7

rcaudy reviewed Jul 15, 2021

View reviewed changes

Review responses

730d0be

devinrsmith requested a review from rcaudy July 16, 2021 00:03

rcaudy approved these changes Jul 16, 2021

View reviewed changes

devinrsmith merged commit 3fb181b into deephaven:main Jul 16, 2021

devinrsmith deleted the table-operations branch July 16, 2021 15:10

nbauernfeind mentioned this pull request Jul 16, 2021

[Java API] Break up Table API #706

Closed

jmao-denver pushed a commit to jmao-denver/deephaven-core that referenced this pull request Apr 11, 2022

Table operations (deephaven#762)

4fea2e5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table operations #762

Table operations #762

devinrsmith commented Jun 17, 2021 •

edited

devinrsmith commented Jun 17, 2021

devinrsmith commented Jun 23, 2021 •

edited

JamesXNelson commented Jun 23, 2021

devinrsmith commented Jun 23, 2021

JamesXNelson left a comment

JamesXNelson Jun 30, 2021

JamesXNelson Jun 30, 2021

devinrsmith Jul 7, 2021

JamesXNelson Jun 30, 2021

devinrsmith Jul 6, 2021

Table operations #762

Table operations #762

Conversation

devinrsmith commented Jun 17, 2021 • edited

devinrsmith commented Jun 17, 2021

devinrsmith commented Jun 23, 2021 • edited

JamesXNelson commented Jun 23, 2021

devinrsmith commented Jun 23, 2021

JamesXNelson left a comment

Choose a reason for hiding this comment

JamesXNelson Jun 30, 2021

Choose a reason for hiding this comment

JamesXNelson Jun 30, 2021

Choose a reason for hiding this comment

devinrsmith Jul 7, 2021

Choose a reason for hiding this comment

JamesXNelson Jun 30, 2021

Choose a reason for hiding this comment

devinrsmith Jul 6, 2021

Choose a reason for hiding this comment

devinrsmith commented Jun 17, 2021 •

edited

devinrsmith commented Jun 23, 2021 •

edited