Refactor Table problem handling to a more robust and hopefully cleaner approach #7879

radeusgd · 2023-09-23T11:30:44Z

Pull Request Description

Closes #7514

Important Notes

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.

…ciated with _operations_ performed on tables, not the data itself

radeusgd · 2023-09-23T12:45:59Z

Note that during the migration we may need some workarounds to tie the old and new problem aggregation methods. These workarounds may not look perfect and they do not show a good representation of how the final approach will look like - it will start to look good once the ProblemAggregator is used all the way down the call chain. Whereas in the first steps, I migrate only some parts and do some conversions to translate between the approaches - these translations are not most pretty, but they allow us to perform the change in an incremental manner.

radeusgd · 2023-09-23T12:53:57Z

First small piece done: I managed to remove getProblems from Table (which was badly tying data to problems about operations) - now some of these operations already rely on the aggregator, altough most of the inner workings still do the handling the old way.

All tests are still passing so no regressions.

We can continue this way, moving stuff piece by piece to the new approach.

This reverts commit 2a9bfbb.

This reverts commit 3216cec

radeusgd · 2023-10-10T17:13:28Z

std-bits/table/src/main/java/org/enso/table/problems/ColumnAggregatedProblemAggregator.java

+  public void reportColumnAggregatedProblem(ColumnAggregatedProblem problem) {
+    for (ColumnAggregatedProblem p : aggregatedProblemList) {
+      if (p.merge(problem)) {
+        // The problem was merged with an existing one.
+        return;
+      }
+    }
+
+    aggregatedProblemList.add(problem);
+  }


ColumnAggregatedProblemAggregator is a bit of a remnant of the old logic.

We would probably have done it slightly differently with this new system. I think we could streamline its internal implementation in the future a little bit - to make it slightly more efficient.

But it currently works well and fits OK into the current architecture, so I thought there is no need to over-engineer this at this point, as I don't think it is a bottleneck by any means - if we find out it is, we can roll more specialized solutions to aggregate the ColumnAggregatedProblem more efficiently.

GregoryTravis

Amazing to see so many classes replaced by a single unified approach!

...table/src/main/java/org/enso/table/data/column/operation/map/MapOperationProblemBuilder.java

jdunkerley

Couple of minor nits buts looks much cleaner albeit with a lot of passing aggregators round a lot more :)

jdunkerley · 2023-10-13T15:33:47Z

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

-        Java_Problems.unpack_value_with_aggregated_problems on_problems problem_mapping=(Parse_Values_Helper.translate_parsing_problem type) <|
-            parser.parseIndependentValue text
+        Java_Problems.with_problem_aggregator on_problems java_problem_aggregator->
+            related_column_name = Nothing


assume this is just for clarity?

Yes - I like to pass named arguments, but Java interop does not allow named args IIRC, so this is a way to show 'which' argument this Null is for.

std-bits/table/src/main/java/org/enso/table/parsing/TypeInferringParser.java

std-bits/table/src/main/java/org/enso/table/parsing/problems/InvalidFormat.java

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

# Conflicts: # distribution/lib/Standard/Table/0.0.0-dev/src/Excel/Excel_Workbook.enso # std-bits/table/src/main/java/org/enso/table/data/column/builder/Builder.java # test/Table_Tests/src/In_Memory/Builders_Spec.enso

radeusgd added 2 commits September 23, 2023 13:02

mark old way as Deprecated

036d7cf

initial implementation of ProblemAggregator (no integrations yet)

92b3b5e

radeusgd added p-low Low priority x-chore Type: chore -libs Libraries: New libraries to be implemented labels Sep 23, 2023

radeusgd assigned radeusgd and GregoryTravis Sep 23, 2023

radeusgd added 3 commits September 23, 2023 14:02

temporary scaffolding to allow easier step-by-step migration

a0e1f44

first success: removed getProblems from Table - problems are now asso…

54e6689

…ciated with _operations_ performed on tables, not the data itself

temporary workaround fix: add a null check

2dbef3c

radeusgd added 18 commits September 28, 2023 12:39

Merge branch 'develop' into wip/table-problem-handling-refactor

3ccbd51

Merge branch 'develop' into wip/table-problem-handling-refactor

d32d1e3

Merge branch 'develop' into wip/table-problem-handling-refactor

f74b7dc

Merge branch 'develop' into wip/table-problem-handling-refactor

1e494a6

remove getProblems from builder - preparing for migration

a4002cb

implement blackhole

2a9bfbb

checkpoint - parse problems, storage builder problems

f3890c8

Revert "implement blackhole"

3216cec

This reverts commit 2a9bfbb.

checkpoint

8f4ac1e

fix

99323ec

remove unused helper

2ba8411

checkpoint

151fa14

rename to be consistent

8ce874b

updating benchmarks

ad6e56d

Revert "Revert "implement blackhole""

ef4b4f2

This reverts commit 3216cec

after revert-revert

e27f07a

avoid 'use after finalize' of aggregators

588bd9b

checkpoint

7e87d0c

radeusgd added 6 commits October 10, 2023 18:33

remove not used anymore AggregatedProblems!

e5507e3

migrate DelimitedReader away from WithProblems

6692611

remove unused WithProblems

309b612

move NameDeduplicator to new approach as well

634b214

remove deprecated scaffolding: reportAll

6539e2b

reformat comment

df1b01d

radeusgd changed the title ~~[DRAFT] Refactoring Table problem handling~~ Refactor Table problem handling to a more robust and hopefully cleaner approach Oct 10, 2023

docs

1397fb0

radeusgd marked this pull request as ready for review October 10, 2023 17:08

radeusgd requested review from jdunkerley and GregoryTravis as code owners October 10, 2023 17:08

radeusgd commented Oct 10, 2023

View reviewed changes

GregoryTravis approved these changes Oct 11, 2023

View reviewed changes

...table/src/main/java/org/enso/table/data/column/operation/map/MapOperationProblemBuilder.java Outdated Show resolved Hide resolved

radeusgd added 5 commits October 11, 2023 23:13

Merge branch 'develop' into wip/table-problem-handling-refactor

7030897

CR: rename MapOperationProblemBuilder to MapOperationProblemAggregator

2d58cdb

follow up the rename

e00ca82

javafmt

c411e16

rename followup 2

fb841ec

jdunkerley approved these changes Oct 13, 2023

View reviewed changes

radeusgd added 5 commits October 16, 2023 13:43

Merge branch 'develop' into wip/table-problem-handling-refactor

3f70262

# Conflicts: # distribution/lib/Standard/Table/0.0.0-dev/src/Excel/Excel_Workbook.enso # std-bits/table/src/main/java/org/enso/table/data/column/builder/Builder.java # test/Table_Tests/src/In_Memory/Builders_Spec.enso

format after merge

b3a3f17

CR2

559d1d9

CR3, docs

7f2c4a4

remove unused variable

ea4dde3

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

radeusgd added 3 commits October 16, 2023 14:25

fixes after merge

cd856b5

add Missing_Required_Argument

a5f724a

Merge branch 'develop' into wip/table-problem-handling-refactor

192c112

mergify bot merged commit 08b717e into develop Oct 16, 2023

mergify bot deleted the wip/table-problem-handling-refactor branch October 16, 2023 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Table problem handling to a more robust and hopefully cleaner approach #7879

Refactor Table problem handling to a more robust and hopefully cleaner approach #7879

radeusgd commented Sep 23, 2023 •

edited

Loading

radeusgd commented Sep 23, 2023

radeusgd commented Sep 23, 2023

radeusgd Oct 10, 2023

GregoryTravis left a comment

jdunkerley left a comment

jdunkerley Oct 13, 2023

radeusgd Oct 16, 2023

Refactor Table problem handling to a more robust and hopefully cleaner approach #7879

Refactor Table problem handling to a more robust and hopefully cleaner approach #7879

Conversation

radeusgd commented Sep 23, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

radeusgd commented Sep 23, 2023

radeusgd commented Sep 23, 2023

radeusgd Oct 10, 2023

Choose a reason for hiding this comment

GregoryTravis left a comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

jdunkerley Oct 13, 2023

Choose a reason for hiding this comment

radeusgd Oct 16, 2023

Choose a reason for hiding this comment

radeusgd commented Sep 23, 2023 •

edited

Loading