Implement `Table.add_row_number` #6890

radeusgd · 2023-05-30T11:17:38Z

Pull Request Description

Closes #5227

Important Notes

This lays first steps towards Remove overhead of calls from Table java code into Enso code by refactoring the functionality to Enso #6292 - we get pure Enso variants of MultiValueKey.
Another part refactors LongStorage into AbstractLongStorage allowing it to provide alternative implementations of the underlying storage, in our case LongRangeStorage generating the values ad-hoc and LongConstantStorage - currently unused but in the future it can be adapted to support constant columns (once we implement similar facilities for other types).

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

radeusgd · 2023-05-30T12:08:13Z

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso

distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Add_Row_Number.enso

radeusgd · 2023-05-31T12:39:42Z

std-bits/table/src/main/java/org/enso/table/data/column/builder/object/DateBuilder.java

-import org.enso.table.data.column.storage.DateStorage;
+import org.enso.table.data.column.storage.datetime.DateStorage;


I moved 'classes' of related storages (datetime and numeric) into sub-packages, as the top-level storage package grew quite a bit and I think this way it will be a bit more manageable.

radeusgd · 2023-05-31T12:42:46Z

...bits/table/src/main/java/org/enso/table/data/column/storage/numeric/LongConstantStorage.java

+package org.enso.table.data.column.storage.numeric;
+
+public class LongConstantStorage extends ComputedLongStorage {
+  private final long constant;
+
+  public LongConstantStorage(long constant, int size) {
+    super(size);
+    this.constant = constant;
+  }
+
+  @Override
+  protected long computeItem(int idx) {
+    return constant;
+  }
+}


This class, together with ComputedLongStorage demonstrates how we can implement O(1) storages for constant columns.

At some point we may do a similar refactor for all other storage types (we should be able to share most of the code for the non-primitive storages, only DoubleStorage will also need special handling to avoid unnecessary boxing, like we are doing here) to support the constant columns better.

jdunkerley

Looks good to me.

The start of refactoring the table is great. I'd like to understand performance of Multi_Value_Key in Enso vs Java.

jdunkerley · 2023-06-01T15:50:42Z

distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso

            Set_Mode.Update -> if self.java_table.getColumnByName renamed.name . is_nothing . not then True else
                Error.throw (Missing_Column.Error renamed.name)


Not really part of this but we could do something using get here. Like:

Suggested change

Set_Mode.Update -> if self.java_table.getColumnByName renamed.name . is_nothing . not then True else

Error.throw (Missing_Column.Error renamed.name)

Set_Mode.Update -> if self.get renamed.name if_missing=(Error.throw (Missing_Column.Error renamed.name))

This will work, but the other branch that is the reversed condition will not work that easily. And I'd prefer to keep the branches symmetric.

jdunkerley · 2023-06-01T15:56:51Z

...bits/table/src/main/java/org/enso/table/data/column/operation/map/numeric/LongBooleanOp.java

-import org.enso.table.data.column.storage.DoubleStorage;
-import org.enso.table.data.column.storage.LongStorage;
-import org.enso.table.data.column.storage.Storage;
+import org.enso.table.data.column.storage.*;


can we avoid *?

Sure, sorry - my formatter sometimes brings these in.

...bits/table/src/main/java/org/enso/table/data/column/operation/map/numeric/LongNumericOp.java

radeusgd requested review from jdunkerley and GregoryTravis as code owners May 30, 2023 11:17

radeusgd force-pushed the wip/radeusgd/6860-add-row-number branch from c3362a0 to 0061994 Compare May 30, 2023 14:24

radeusgd linked an issue May 30, 2023 that may be closed by this pull request

Implement the add_row_number function #6860

Closed

radeusgd self-assigned this May 30, 2023

radeusgd mentioned this pull request May 30, 2023

Remove overhead of calls from Table java code into Enso code by refactoring the functionality to Enso #6292

Open

GregoryTravis reviewed May 30, 2023

View reviewed changes

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso Outdated Show resolved Hide resolved

GregoryTravis approved these changes May 30, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/6860-add-row-number branch from 0061994 to e52a4b3 Compare May 31, 2023 12:38

radeusgd commented May 31, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/6860-add-row-number branch from e52a4b3 to e7a94c4 Compare May 31, 2023 12:47

GregoryTravis approved these changes May 31, 2023

View reviewed changes

jdunkerley approved these changes Jun 1, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/6860-add-row-number branch from e7a94c4 to 9a5bc94 Compare June 1, 2023 17:53

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jun 1, 2023

radeusgd added 13 commits June 2, 2023 11:14

initial stuff

28c882d

adding tests

6b47b89

fix names, more tests

182d1e6

the general framework, simple case passing

9745be4

Implement grouping. First steps towards #6292

4efd4d5

split MVI into 2 again, add LongArray and create the sorted variant

a1b5361

add a test, fix a typo

3508fdd

floating point grouping warnings

1f0d905

changelog

37ea2e5

docs

5351a7c

widgets

d5f1ad7

Add LongRangeStorage

8feee79

move stuff to more packages for tidiness

9083719

radeusgd added 5 commits June 2, 2023 11:15

fix access

0ec7bec

some abstraction, demo a LongConstantStorage

4fc28b2

javafmt

8f06564

CR

11d4196

fmt

7558c56

radeusgd force-pushed the wip/radeusgd/6860-add-row-number branch from 9a5bc94 to 7558c56 Compare June 2, 2023 09:22

mergify bot merged commit d44b125 into develop Jun 2, 2023

mergify bot deleted the wip/radeusgd/6860-add-row-number branch June 2, 2023 10:13

radeusgd mentioned this pull request Aug 11, 2023

union of two tables reports an "unexpected storage implementations" error when add_row_number precedes the union #7565

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `Table.add_row_number` #6890

Implement `Table.add_row_number` #6890

radeusgd commented May 30, 2023 •

edited

Loading

radeusgd commented May 30, 2023

radeusgd May 31, 2023

radeusgd May 31, 2023

jdunkerley left a comment

jdunkerley Jun 1, 2023

radeusgd Jun 1, 2023

jdunkerley Jun 1, 2023

radeusgd Jun 1, 2023

		import org.enso.table.data.column.storage.DateStorage;
		import org.enso.table.data.column.storage.datetime.DateStorage;

		Set_Mode.Update -> if self.java_table.getColumnByName renamed.name . is_nothing . not then True else
		Error.throw (Missing_Column.Error renamed.name)

	Set_Mode.Update -> if self.java_table.getColumnByName renamed.name . is_nothing . not then True else
	Error.throw (Missing_Column.Error renamed.name)
	Set_Mode.Update -> if self.get renamed.name if_missing=(Error.throw (Missing_Column.Error renamed.name))

Implement Table.add_row_number #6890

Implement Table.add_row_number #6890

Conversation

radeusgd commented May 30, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

radeusgd commented May 30, 2023

radeusgd May 31, 2023

Choose a reason for hiding this comment

radeusgd May 31, 2023

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

jdunkerley Jun 1, 2023

Choose a reason for hiding this comment

radeusgd Jun 1, 2023

Choose a reason for hiding this comment

jdunkerley Jun 1, 2023

Choose a reason for hiding this comment

radeusgd Jun 1, 2023

Choose a reason for hiding this comment

Implement `Table.add_row_number` #6890

Implement `Table.add_row_number` #6890

radeusgd commented May 30, 2023 •

edited

Loading