Initial Tableau Reading Support #10733
Conversation
The main problem was that Classloader used during HyperAPI was incorrect We should be using the one that loaded the class, which was a rather straightforward change. Additionally we must set correct permissions on `hyperd`. sbt will attempt to download necessary dependenciess to `unmanagedClasspath` if jars are missing.
jdunkerley
added
the
CI: Clean build required
Add ICON and GROUP to Hyper_File.
Should make it more resilient to incremental compilation steps that appear on CI. Also reproducible locally when `lib`'s contents is removed manually.
build.sbt
Outdated
| fetchZipToUnmanaged := { | ||
| val unmanagedDirectory = (Compile / unmanagedBase).value | ||
| if (IO.listFiles(unmanagedDirectory).size < 2) { // Heuristic, should have at least hyperapi jar and os-specific one. | ||
| System.out.println( |
There was a problem hiding this comment.
In general I didn't see the need for any logging. Somehow CI doesn't like my solution and we can't reproduce locally
There was a problem hiding this comment.
As you prefer. I think it's useful to see a log for this (at least on debug level) to be able to tell what's happening. I'd especially appreciate a message right before we start a download of the 200Mb file - for fiber connections that's moments, but when you are developing on a train, getting a message explaining to you why the build process just started to block for 5 minutes is actually precious information.
I think it's better to do this via ManagedLogger though, as it allows users to manage log levels and gathers logs in one place - print is going to be completely separate from it.
| 'jna-platform', licensed under the Apache-2.0, is distributed with the Tableau. | ||
| The license file can be found at `licenses/APACHE2.0`. | ||
| Copyright notices related to this dependency can be found in the directory `net.java.dev.jna.jna-platform-5.14.0`. | ||
|
|
There was a problem hiding this comment.
Forgot to say but I think ideally we should add an entry about HyperAPI here. Can be added by modifying notice-header.
But current is probably OK too since we do have the COPYRIGHT-HYPERAPI file, so feel free to ignore.
distribution/lib/Standard/Tableau/0.0.0-dev/src/Hyper_Table.enso
Outdated
Show resolved
Hide resolved
| ## ICON metadata | ||
| Returns the name of the current schema. | ||
| `*` represents all schemas. | ||
| schema : Text | ||
| schema self = self.internal_schema |
There was a problem hiding this comment.
What does it mean if f.schema returns "*"?
There was a problem hiding this comment.
It means accessing all tables - schema isn't used much inside Hyper but it is in theory supported.
std-bits/tableau/src/main/java/org/enso/tableau/HyperReader.java
Outdated
Show resolved
Hide resolved
| // Check if any files in the hyper directory, otherwise download them. | ||
| try (var files = Files.list(HYPER_PATH)) { | ||
| if (files.findAny().isEmpty()) { | ||
| switch (OSPlatform.CurrentPlatform) { | ||
| case WINDOWS -> downloadHyper( | ||
| "https://enso-data-samples.s3.us-west-1.amazonaws.com/tableau/hyperd.exe", | ||
| "hyperd.exe", | ||
| false); | ||
| case MAC_ARM64 -> downloadHyper( | ||
| "https://enso-data-samples.s3.us-west-1.amazonaws.com/tableau/macos-arm64/hyperd", | ||
| "hyperd", | ||
| true); | ||
| case MAX_X64 -> throw new IOException( | ||
| "Unsupported platform: Only ARM64 Mac is supported."); | ||
| case LINUX -> downloadHyper( | ||
| "https://enso-data-samples.s3.us-west-1.amazonaws.com/tableau/linux/hyperd", | ||
| "hyperd", | ||
| true); | ||
| case OTHER -> throw new IOException( | ||
| "Unsupported platform: " + OSPlatform.CurrentPlatform); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why cannot this executable be distributed together with the library?
There was a problem hiding this comment.
It's massive (>200Mb) so making it fetch it when used.
There was a problem hiding this comment.
Okay, fine for now but I think for longer term we should prefer to keep this as part of the library and try using our existing infrastructure to fetch the whole library on demand.
Also that's why I'd like to put this into a cache directory - if we ever switch to distributing with library, a cache dir will get more likely to be cleaned up, while this may stay on user's system for longer.
std-bits/tableau/src/main/java/org/enso/tableau/OSPlatform.java
Outdated
Show resolved
Hide resolved
std-bits/tableau/src/main/java/org/enso/tableau/TableColumnBuilder.java
Outdated
Show resolved
Hide resolved
jdunkerley
requested review from
GregoryTravis,
AdRiley,
marthasharkey,
4e6,
JaroslavTulach,
hubertp and
Akirathan
as code owners
Handle missing file.
Previously the task was reporting an empty sequence despite fetching the required jars.
| @schema (hyper -> make_schema_selector hyper True) | ||
| set_schema : Text -> Hyper_File | ||
| set_schema self schema = | ||
| if schema == self.schema then self else |
There was a problem hiding this comment.
Is schema a collection of tables, or is it table metadata?
Since our CI builds things multiple times and it might happen `unmanagedClasspath` was not triggered before `unmanagedJars`, we could have a situation when necessary jars were not copied to the distribution. This change ensures that is no longer the case.
Co-authored-by: GregoryTravis <[email protected]>
Pull Request Description
Hyper_Fileallowing reading a Tableau hyper file.Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.