Reworking Excel support to allow for reading of big files #8403

radeusgd · 2023-11-27T14:59:50Z

Pull Request Description

Closes Reading a large Excel file resulted in an error about number of records #8111 by making sure that all Excel workbooks are read using a backing file (which should be more memory efficient).
- If the workbook is being opened from an input stream, that stream is materialized to a Temporary_File.
Adds tests fetching Table formats from HTTP.
- Extends simple-httpbin with ability to serve files for our tests.
Ensures that the Infer option on Excel format also works with streams, if content-type metadata is available (e.g. from HTTP headers).
Implements a Temporary_File facility that can be used to create a temporary file that is deleted once all references to the Temporary_File instance are GCed.

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.

radeusgd · 2023-11-28T19:50:14Z

build.sbt

@@ -2717,11 +2717,12 @@ val allStdBits: Parser[String] =
 lazy val `simple-httpbin` = project
  .in(file("tools") / "simple-httpbin")
  .settings(
-    frgaalJavaCompilerSetting,


The frgaal compiler settings have been causing compile error:
cannot find symbol import com.sun.net.httpserver.SimpleFileServer: cannot find symbol.

Apparently SimpleFileServer may not be in the JDK that Frgaal uses? Weird.

Anyway there is no benefit in Frgaal within this sub-project, especially as we are already on JDK21. Removing it allowed me to re-use the SimpleFileServer for serving files for testing, instead of having to roll my own solution or having to bring in some heavy dependencies.

Frgaal by default only exposes only classes that are in API of the JDK. SimpleFileServer isn't.

no benefit in Frgaal within this sub-project

There is a benefit. It already prevented you from using SimpleFileServer!

Frgaal by default only exposes only classes that are in API of the JDK. SimpleFileServer isn't.

no benefit in Frgaal within this sub-project

There is a benefit. It already prevented you from using SimpleFileServer!

I really honestly don't understand. HOW is that a benefit?

Without Frgaal the server is running fine on our JDK. What is the benefit of using it this translation layer at this point?

We introduced it to be able to use newer features before we migrated to the newer JDKs. But now we have these features natively, so what else?

server is running fine on our JDK.

You have compiled your code in a way that allows running it on JDK 11. However your code cannot run on JDK 11. Without Frgaal you wouldn't even notice that your project is misconfigured as it requires JDK18+ to run.

server is running fine on our JDK.

You have compiled your code in a way that allows running it on JDK 11. However your code cannot run on JDK 11. Without Frgaal you wouldn't even notice that your project is misconfigured as it requires JDK18+ to run.

Hm, I see. It was hard to appreciate that, because from my perspective, removing Frgaal made it work - I never really specifically wanted to compile simple-httpbin to run on JDK 11 - just the current JDK we use - especially since it's not even an artifact we ship but just an internal test scaffolding.

… memory - allowing to handle big files more efficiently WIP: ensuring the resources are closed when we finish using a workbook

…lexibility in how it is performed TODO: update Enso part

…nsuring it works even if the files were held open

…d then open the temp file for writing

… nonexisting

…the package

# Conflicts: # distribution/lib/Standard/Test/0.0.0-dev/src/Test.enso

radeusgd · 2023-12-07T18:16:10Z

Can I please ask someone from engine to approve the build.sbt changes? cc: @Akirathan @hubertp @JaroslavTulach

JaroslavTulach

Don't blame frgaal. Compile against appropriate -target JDK APIs.

JaroslavTulach · 2023-12-08T05:19:49Z

build.sbt

@@ -2717,11 +2717,12 @@ val allStdBits: Parser[String] =
 lazy val `simple-httpbin` = project
  .in(file("tools") / "simple-httpbin")
  .settings(
-    frgaalJavaCompilerSetting,


Frgaal by default only exposes only classes that are in API of the JDK. SimpleFileServer isn't.

no benefit in Frgaal within this sub-project

There is a benefit. It already prevented you from using SimpleFileServer!

tools/simple-httpbin/src/main/java/org/enso/shttp/SimpleHTTPBin.java

JaroslavTulach

By default we should use -target 11 - to support for example the #8094 effort.

JaroslavTulach · 2023-12-08T15:08:59Z

build.sbt

@@ -1330,13 +1330,16 @@ lazy val truffleDslSuppressWarnsSetting = Seq(
 )

 /** A setting to replace javac with Frgaal compiler, allowing to use latest Java features in the code
-  * and still compile down to JDK 11
+  * and still compile down to JDK 17


Why do we change the -target level to 17?

I don't think there is any sophisticated reason for that. I just thought that it was a good idea to bump that to -target 17. Furthermore, I vaguely remember that we talked about this as well. But I think we can fall back to 11.

I have only updated the documentation comment to be aligned with what is actually being done.

# Conflicts: # CHANGELOG.md

jdunkerley · 2023-12-14T15:40:39Z

...-sources/enso-test-java-helpers/src/main/java/org/enso/table_test_helpers/RandomHelpers.java

+
+import java.util.Random;
+
+public class RandomHelpers {


Can't use the Faker here?

We could, but I was annoyed with how slow it was:

from Standard.Base import all from Standard.Test import Faker polyglot java import org.enso.table_test_helpers.RandomHelpers main = n = 10^6 faker = Faker.new 123 r1 = Duration.time_execution <| Vector.new n _-> faker.alpha_numeric 190 IO.println <| " Faker: " + r1.first.to_display_text rng = RandomHelpers.new 123 r2 = Duration.time_execution <| Vector.new n _-> rng.makeRandomString 190 IO.println <| "RandomHelpers: " + r2.first.to_display_text

Faker: 15.3227068s RandomHelpers: 01.7083387s

Almost 10x slower. I'll merge as-is but happy to replace back with Faker, as the duplication is not ideal. Maybe we should try to improve the Faker's performance? I think the current way has a lot of overhead.

This reverts commit 3ea7805.

radeusgd force-pushed the wip/radeusgd/8111-big-excel-fix branch 2 times, most recently from 89ff416 to 2289c62 Compare November 28, 2023 16:48

radeusgd commented Nov 28, 2023

View reviewed changes

radeusgd added 27 commits November 29, 2023 13:33

adding a test for loading big Excel files

c8e2b40

change how random file is generated

8a34596

adapt tests to work for XLS

c6d8a37

open workbook as file, instead of materializing whole input stream in…

cd26b95

… memory - allowing to handle big files more efficiently WIP: ensuring the resources are closed when we finish using a workbook

checkpoint

e5c1bf0

add a test

3003bbd

checkpoint - ExcelConnectionPool

5a9929f

checkpoint - moving logic to use the pool

11be28e

checkpoint

13d4009

checkpoint 2

9a4e4c0

amend test

02e76c2

checkpoint 3

ad8b60d

checkpoint 4

db2b4e3

test reopening file, change exception on wrong format open

17e2ab4

fixing some edge cases: use after close, headers in a re-read test

3f35168

implement empty overwrite test

fdf4f41

checkpoint

deed106

temporary workaround for #8384

1e8502f

fixing smol stuff

408d5bf

fix

6533c38

workaround for the XLSX append

cf5267b

checkpoint: change the pool logic for writing, giving the user more f…

72f9517

…lexibility in how it is performed TODO: update Enso part

implement custom Excel writing logic - dry run + backup + tests for e…

38e957a

…nsuring it works even if the files were held open

fixes to get it compiling

9bf9b14

writing a read-only workbook fails, trying another strategy - copy an…

4b80e9f

…d then open the temp file for writing

update some tests

b1d6a92

remove canonical - it worked only for existing files but we also need…

464a3f6

… nonexisting

radeusgd added 12 commits December 1, 2023 02:52

javafmt

8853469

changelog

c5cd63f

update prettier ignore

0f45573

amendments to HTTP - some optional logging, not include Scala lib in …

a147278

…the package

CR, pending TODO

3b1ea01

update test

d5d83a1

Merge branch 'develop' into wip/radeusgd/8111-big-excel-fix

15ecad5

Merge branch 'develop' into wip/radeusgd/8111-big-excel-fix

9a22d3a

httpbin log stop of server

ddc3b2e

javafmt

c5f99c9

try removing the Press Enter to stop in hope of fixing CI

4b5e589

Merge branch 'develop' into wip/radeusgd/8111-big-excel-fix

f00d2d3

# Conflicts: # distribution/lib/Standard/Test/0.0.0-dev/src/Test.enso

radeusgd mentioned this pull request Dec 7, 2023

Improving tests and edge cases for URI and HTTP #8497

Merged

5 tasks

JaroslavTulach requested changes Dec 8, 2023

View reviewed changes

radeusgd added 2 commits December 8, 2023 13:45

Merge branch 'develop' into wip/radeusgd/8111-big-excel-fix

e427758

CR: bring back frgaal and change target JDK for simple-httpbin

fa11eab

radeusgd requested a review from JaroslavTulach December 8, 2023 13:18

JaroslavTulach approved these changes Dec 8, 2023

View reviewed changes

radeusgd added 2 commits December 12, 2023 12:05

Merge branch 'develop' into wip/radeusgd/8111-big-excel-fix

10ab3e1

Merge branch 'develop' into wip/radeusgd/8111-big-excel-fix

a4ec0e0

# Conflicts: # CHANGELOG.md

jdunkerley approved these changes Dec 14, 2023

View reviewed changes

radeusgd added 3 commits December 14, 2023 23:36

use Faker instead of other helpers

3ea7805

Revert "use Faker instead of other helpers"

858b1b4

This reverts commit 3ea7805.

remove obsolete comment

0747c08

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

radeusgd self-assigned this Dec 14, 2023

mergify bot merged commit b5c995a into develop Dec 15, 2023
31 checks passed

mergify bot deleted the wip/radeusgd/8111-big-excel-fix branch December 15, 2023 00:02

radeusgd mentioned this pull request Dec 15, 2023

Unify handling of file format detection #8555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworking Excel support to allow for reading of big files #8403

Reworking Excel support to allow for reading of big files #8403

radeusgd commented Nov 27, 2023 •

edited

Loading

radeusgd Nov 28, 2023

JaroslavTulach Dec 8, 2023

radeusgd Dec 8, 2023

JaroslavTulach Dec 8, 2023

radeusgd Dec 8, 2023

radeusgd commented Dec 7, 2023 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Dec 8, 2023

JaroslavTulach left a comment

JaroslavTulach Dec 8, 2023

Akirathan Dec 8, 2023

radeusgd Dec 8, 2023

jdunkerley Dec 14, 2023

radeusgd Dec 14, 2023

Reworking Excel support to allow for reading of big files #8403

Reworking Excel support to allow for reading of big files #8403

Conversation

radeusgd commented Nov 27, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

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 Dec 7, 2023 • edited Loading

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach 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 Nov 27, 2023 •

edited

Loading

radeusgd commented Dec 7, 2023 •

edited

Loading