Add appending support for Delimited files #3573

radeusgd · 2022-07-07T18:07:42Z

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/182309839

Important Notes

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide dist and ./run ide watch.

hubertp

nits

hubertp · 2022-07-11T08:03:04Z

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

+## PRIVATE
+   Reads the beginning of the file to detect the existing headers and column
+   count.
+detect_headers : File -> File_Format.Delimited -> Detected_Headers


It kinda feels like this should use a Maybe and be empty instead of using a separate new marker type No_Data

Hmm, I thought it makes sense to create a domain-specific ADT for all the cases here. But your solution also sounds good to me, so I can switch to it. I think I'd use a Nothing as we know it would either be a Nothing or one of the two other ADT cases - using Maybe seems like an overkill to me because I want to represent 3 states and not a presence or lack of two states.

hubertp · 2022-07-11T08:21:16Z

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

+should_write_headers headers = case headers of
+    True -> True
+    Infer -> True
+    False -> False


Suggested change

should_write_headers headers = case headers of

True -> True

Infer -> True

False -> False

should_write_headers headers = case headers of

Infer -> True

_ -> headers

?

Yeah, well it can also be:

case headers of False -> False _ -> True

but I wanted to make it explicit which of the three options map to which. IMO the codes where the other two options are collapsed are slightly less readable - it is less clear which options map to which.

std-bits/table/src/main/java/org/enso/table/read/DelimitedReader.java

hubertp · 2022-07-11T08:25:37Z

std-bits/table/src/main/java/org/enso/table/read/DelimitedReader.java

+    if (effectiveColumnNames == null) {
+      detectHeaders();
+    }
+


Looks like ensureHeadersDetected could be called here instead for DRY?

hubertp · 2022-07-11T08:27:06Z

std-bits/table/src/main/java/org/enso/table/read/DelimitedReader.java

+    skipFirstRows();
+    Row firstRow = internalReadNextRow();
+    if (firstRow == null) {
+      effectiveColumnNames = new String[0];


shouldn't this return null according to the comment on getDefinedColumnNames?

effectiveColumnNames and definedColumnNames are two distinct concepts.

test/Table_Tests/data/.gitignore

jdunkerley

Minor comments but looks good to me.

jdunkerley · 2022-07-11T08:31:37Z

std-bits/table/src/main/java/org/enso/table/read/DelimitedReader.java

+
+  private record Row(long lineNumber, String[] cells) {}
+
+  private final Deque<Row> pendingRows = new ArrayDeque<>(2);


This can be a Stack

How so? I need it to be FIFO, but stack is FILO, no? Or am I missing something?

Sorry meant a queue. We don't need the double ended version.

Ah, makes sense then.

Still - I don't know a Queue interface implementation in Java's stdlib that is not also a Deque (apart from the BlockingQueue but we certainly don't need the concurrency features here).

Ah, but I can make the type Queue while still instantiating the ArrayDeque. That sounds good.

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

to run it without reading the whole file - that will be used by Delimited Append.

radeusgd self-assigned this Jul 7, 2022

radeusgd changed the base branch from develop to wip/radeusgd/error-catch-by-type July 8, 2022 15:02

radeusgd force-pushed the wip/radeusgd/delimited-append-182309839 branch from c980a79 to 4f8aca5 Compare July 8, 2022 15:02

radeusgd force-pushed the wip/radeusgd/error-catch-by-type branch from 3736803 to 5d3ae84 Compare July 8, 2022 16:22

radeusgd force-pushed the wip/radeusgd/delimited-append-182309839 branch from 00242dd to 9abbb56 Compare July 8, 2022 17:26

radeusgd marked this pull request as ready for review July 8, 2022 17:27

radeusgd requested review from 4e6 and jdunkerley as code owners July 8, 2022 17:27

radeusgd requested a review from hubertp July 8, 2022 17:29

Base automatically changed from wip/radeusgd/error-catch-by-type to develop July 11, 2022 08:26

mergify bot requested a review from JaroslavTulach as a code owner July 11, 2022 08:26

hubertp approved these changes Jul 11, 2022

View reviewed changes

jdunkerley approved these changes Jul 11, 2022

View reviewed changes

radeusgd added 9 commits July 11, 2022 10:53

Refactor DelimitedReader to split the header discovery logic to be able

326176f

to run it without reading the whole file - that will be used by Delimited Append.

Add gitignore

728b097

checkpoint

f69ea1d

Implemented core append logic

608fa37

adding tests, fixes

abd899c

Fix empty file handling

b0c0004

Append By_Name tests

c25e30e

Add tests for positional append, mismatches

92e4c2c

Rename Table.Error to Table.Errors, fix tests

12b0e42

radeusgd force-pushed the wip/radeusgd/delimited-append-182309839 branch from 9abbb56 to 12b0e42 Compare July 11, 2022 08:53

radeusgd added 4 commits July 11, 2022 11:36

cr

4500bc0

cr 2

c171ff1

changelog

0a4f8bc

cleanup after tests

a9db052

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jul 11, 2022

mergify bot added 2 commits July 11, 2022 10:30

Merge branch 'develop' into wip/radeusgd/delimited-append-182309839

9371302

Merge branch 'develop' into wip/radeusgd/delimited-append-182309839

a988b86

mergify bot merged commit df10e4b into develop Jul 11, 2022

mergify bot deleted the wip/radeusgd/delimited-append-182309839 branch July 11, 2022 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add appending support for Delimited files #3573

Add appending support for Delimited files #3573

radeusgd commented Jul 7, 2022 •

edited

Loading

hubertp left a comment

hubertp Jul 11, 2022

radeusgd Jul 11, 2022

hubertp Jul 11, 2022

radeusgd Jul 11, 2022

hubertp Jul 11, 2022

hubertp Jul 11, 2022

radeusgd Jul 11, 2022

jdunkerley left a comment

jdunkerley Jul 11, 2022

radeusgd Jul 11, 2022

jdunkerley Jul 11, 2022

radeusgd Jul 11, 2022

radeusgd Jul 11, 2022


		private record Row(long lineNumber, String[] cells) {}

		private final Deque<Row> pendingRows = new ArrayDeque<>(2);

Add appending support for Delimited files #3573

Add appending support for Delimited files #3573

Conversation

radeusgd commented Jul 7, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

hubertp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd commented Jul 7, 2022 •

edited

Loading