Demonstrate usage of foreign arrow function #9150

hubertp · 2024-02-23T05:42:32Z

Pull Request Description

Including arrow language in the distribution by default. Added a basic example for creating an Arrow array.
Making sure that memory layout agrees with Arrow specification (padding, continuous allocation of memory chunks).
Related to #9118.

This should unblock work on allowing serialization/deserialization to/from Parquet but I'd like to delay it to a follow up ticket as it is going to be a significant amount of specialized work.

Important notes

As requested, Arrow array can currently be only modified via a builder pattern. Once the builder is sealed, no more modifications are allowed. At least not via official API. We all know that one can bypass those via a foreign function in JS or Python.

Checklist

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

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.

Including arrow language in the distribution by default. Added a basic example for creating an Arrow array.

Akirathan · 2024-02-23T09:51:24Z

build.sbt

@@ -3021,7 +3020,8 @@ buildEngineDistribution := {
  val _ = (`engine-runner` / assembly).value
  updateLibraryManifests.value
  val modulesToCopy = componentModulesPaths.value.map(_.data)
-  val engineModules = Seq(file("runtime.jar"))
+  val arrow = Seq((`runtime-language-arrow`/Compile/packageBin ).value)


I think this is correct. Let's not assemble runtime-language-arrow, as that is unnecessary.

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

Arrow specification will enforce padding during serialization. This change is mostly copying from existing Java reference implementation. Also while the padding is now correct, allocation of separate `ByteDirect` buffers is not. TBC

The builder accepts an Arrow array and allows for populating the array via the `set` method. Once the builder is `built` no more modifications are allowed.

Arrow spec expects (informally) that memory allocated to data and bitmaps is continous. With this change, when allocating memory for some array, we allocate full chunk of memory and then slice **from it** chunks for data and bitmap, rather than allocating separate memory chunks. TODO: add some reference counting in the future.

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Arrow_Array_Builder.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Polyglot.enso

...java/org/enso/interpreter/node/expression/builtin/interop/generic/WriteArrayElementNode.java

...src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SetBuiltinVectorNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrowArrayBuilder.java

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

JaroslavTulach

We have designed, implemented and packaged the Arrow language as a separate, Enso independent entity. This PR puts Arrow Array Builder into Standard.Base. That's not right!

Arrow isn't a core concept of Enso. As such there should be no builtins related to Arrow support.

Should the Arrow language remain independent from Enso language, then the builder has to be provided by the Arrow language, exposing its functionality via InteropLibrary. Simplest idea is to require two arguments to new_arrow constructor (created after parsing new[Int8] by the Arrow language) - the first being the size and the second an isExecutable TruffleObject (for example an Enso lambda function) that will be execute(builder) where builder is another TruffleObject reacting to invokeMember("set") & co.

Or we can decide that Arrow is natural part of Enso. But then we shouldn't have a separate Arrow language JAR & co. Needless to say, I'd rather keep Enso small and have Arrow separate - unless there are inherent performance or UX limitations.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Arrow_Array_Builder.enso

...java/org/enso/interpreter/node/expression/builtin/interop/generic/WriteArrayElementNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrowArrayBuilder.java

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

hubertp · 2024-03-06T09:43:23Z

We have designed, implemented and packaged the Arrow language as a separate, Enso independent entity. This PR puts Arrow Array Builder into Standard.Base. That's not right!

Despite the name, there is nothing Arrow-specific in that module. And I think it would make sense to make it generic. I will update the PR.

hubertp · 2024-03-06T11:29:47Z

@JaroslavTulach As I mentioned, there was nothing in the builder that made it arrow-specific. In fact this can serve as a generic polyglot array builder. The changes now reflect that.
But I also added a case where you can use Polyglot API to do the modifications in place. Then the sky is the limit (the potential damage as well).

hubertp · 2024-03-06T11:30:44Z

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

+foreign arrow new_arrow = """
+    new[Int8]
+
+Polyglot_Array_Builder.set self index value = Polyglot_Array_Builder.set_builtin self index value


I can remove this if people object. This is mostly for testing the functionality and it is not part of the official API

hubertp · 2024-03-06T11:33:25Z

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

+            builder.set 1 0 . should_fail_with Unsupported_Argument_Types
+            Polyglot.write_array_element array_10 1 0
+            builder.at 1 . should_equal 0
+            v.at 1 . should_equal 0 # dangerous but allowed


@JaroslavTulach This demonstrates the potential consequences of using Polyglot API i.e. updating the builder after the fact that makes it "sealed".
/cc @jdunkerley

demonstrates the potential consequences of using Polyglot API

This demonstrates that the sealing only happens in the builder, but not on the actual array. If we want object coming from our arrow language to support sealing, why don't we put the sealed flag into ArrowFixedArrayInt & co.?

JaroslavTulach

I continue to have an API design architectural problem with this PR.

was nothing in the builder that made it arrow-specific. In fact this can serve as a generic polyglot array builder. The changes now reflect that.

Whatever was @jdunkerley reason for requesting the builder, it is not achieved as the usage of the new builder is completely optional and there are other means to achieve the same (and more) without the builder.

Proper API design shall start with description of the use-case. As a reviewer I am not supposed to guess the use-case. However I have to: If the use-case was encapsulation of the write operation, then it is not achieved! If the use-case is something else, then please mention it in the Pull Request Description - as currently the justification for majority of the code (I mean the bloat related to all the new builder builtins) in this PR is missing (or at last I cannot find it).

But I also added a case where you can use Polyglot API to do the modifications in place. Then the sky is the limit (the potential damage as well).

Right. So what is your use-case related to Polyglot.write_array_element? Should one be allowed to do the modifications in place from Enso language or not? The original decision when working on #5011 and related PRs was that we don't want to expose write operations in Enso.

Using builder pattern is a good way to avoid exposing write operations, however it has to be done properly - e.g. (in current architectural design) the builder has to be provided by the Arrow language.

JaroslavTulach · 2024-03-07T05:36:49Z

...ne/runtime-language-arrow/src/main/java/org/enso/interpreter/arrow/runtime/RoundingUtil.java

+
+package org.enso.interpreter.arrow.runtime;
+
+public class RoundingUtil {


Does the class have to be public at all? Isn't it enough to be package private?

JaroslavTulach · 2024-03-07T05:39:24Z

...java/org/enso/interpreter/node/expression/builtin/interop/generic/WriteArrayElementNode.java

+      err.enter();
+      Builtins builtins = EnsoContext.get(this).getBuiltins();
+      throw new PanicException(builtins.error().makeInvalidArrayIndex(array, index), this);
+    } catch (UnsupportedTypeException e) {


UnsupportedMessageException leads to makeTypeError while UnsupportedTypeException leads to panic. Isn't that strange? Shouldn't UnsupportedTypeException lead to some type error?

JaroslavTulach · 2024-03-07T05:44:04Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java

@@ -165,6 +168,10 @@ public Atom makeNoSuchConversion(Object target, Object that, UnresolvedConversio
    return noSuchConversion.newInstance(target, that, conversion);
  }

+  public Atom makeInvalidOperation(Object message) {


The Object message type is pretty generic and potentially wrong. We cannot pass java.lang.String there - that'd be an error. Recently I changed one of such signatures to Text. Other alternative is EnsoObject or TruffleObject or overloaded methods supporting also long and double.

I can see the Object argument is accepted all around, but as I say, it is technically not correct.

JaroslavTulach · 2024-03-07T05:45:13Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/PolyglotArrayBuilder.java

+    stdlibName = "Standard.Base.Data.Polyglot_Array_Builder.Polyglot_Array_Builder")
+public abstract class PolyglotArrayBuilder implements EnsoObject {
+
+  protected boolean sealed = false;


protected? Better to make (package) private.

JaroslavTulach · 2024-03-07T05:45:22Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/PolyglotArrayBuilder.java

+
+  protected boolean sealed = false;
+
+  public void seal() {


JaroslavTulach · 2024-03-07T05:51:05Z

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

+            builder.set 1 0 . should_fail_with Unsupported_Argument_Types
+            Polyglot.write_array_element array_10 1 0
+            builder.at 1 . should_equal 0
+            v.at 1 . should_equal 0 # dangerous but allowed


demonstrates the potential consequences of using Polyglot API

This demonstrates that the sealing only happens in the builder, but not on the actual array. If we want object coming from our arrow language to support sealing, why don't we put the sealed flag into ArrowFixedArrayInt & co.?

JaroslavTulach · 2024-03-07T06:00:43Z

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

+            v.at 8 . should_equal 127
+            builder.append 21 . should_fail_with Invalid_Operation
+            builder.set 1 21 . should_fail_with Invalid_Operation
+            Polyglot.write_array_element array_10 1 0


Let me recap what we achieved so far:

we expose new[Int8] to public

we let public to create array_10 and hold it indefinitly

we let public to modify the array any time with Polyglot.write_array_element array_10

on top of that we created some fake facade called Polyglot_Array_Builder

the facade provides no additional functionality

every operation can already be done on array_10 by other means

I guess I am missing the point of the whole builder exercise. Whatever it tried to achieve (probably some encapsulation or giving the creator more rights) clearly isn't achieved. Btw. Give the Creator of an Object More Rights pattern is described at page 79 of the Practical API Design book.

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrowArrayBuilder.java

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

JaroslavTulach · 2024-03-07T09:35:32Z

Assuming @jdunkerley main requirement is the: don't let mutable arrays float thru the computation, then ...

.... Simplest idea is to ....

Change the syntax of the Arrow language to create the builder itself:

foreign arrow new_array_int8 = """
    new[Int8]

b10 = new_array_int8 10
b10.append 2 
b10.append 4
arr = b10.build # or to_array?

arr.length . should_equal 10
arr.at 0 . should_equal 2
arr.at 1 . should_equal 4
arr.at 5 . is_nothing . should_be_true # maybe?

Polyglot.write_array_element arr 6 33 # raises an exception as the arr is already immutable

if this is what we want, then it all can be implemented in the Arrow language itself without any need for so many builtins in the Enso engine. As a result of the changes the test cases that use new would need to be updated to use the builder (newly returned from the factory method).

Arrow Array is already Enso Array

Please note that one can apply all Enso Array methods on the arr produced by Arrow language:

b3 = new_array_int8 3
b3.append 1 
b3.append 2
b3.append 3
arr = b3.build 

v1 = arr.map _.to_text # yields ['1', '2', '3']
v2 = arr.fold 10 (+) # yields 16
v3 = Meta.type_of arr # yields Array

Avoiding the introduction of various builtins by pushing builder pattern into allow language.

JaroslavTulach

From an architecture perspective the change is looking good now. It is located in the right place (e.g. runtime-language-arrow), it doesn't require any changes to engine/runtime. It fits perfectly the vision why Arrow language was founded as a (loosely) separated project.

I have doubts about the decision to make builder : Array. Possibly that's unnecessary leakage of mutability into the Enso world. However that may stay as it is and remain a subject of future discussions.

I believe we should improve error reporting by using better subclasses of InteropException. Given the typed array nature of Arrow buffers, I'd use UnsupportedTypeException more.

I haven't verified the change from the point of performance. I assume there will be some benchmarking work in the future.

...ime-language-arrow/src/main/java/org/enso/interpreter/arrow/runtime/ArrowFixedArrayDate.java

JaroslavTulach · 2024-03-08T04:21:32Z

...guage-arrow/src/main/java/org/enso/interpreter/arrow/runtime/ArrowFixedSizeArrayBuilder.java

+
+  @ExportMessage
+  public boolean hasArrayElements() {
+    return true;


The "builder" is in fact also an (Enso) Array and it can be mutated until somebody calls build on it. I'd be interested to see a use-cases for such a design decision...

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

...guage-arrow/src/main/java/org/enso/interpreter/arrow/runtime/ArrowFixedSizeArrayBuilder.java

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso

Follow up on #9150 - making sure that Arrow builder is not accidentally treated as an Array by disallowing reading elements. # Important Notes Also making sure that the length of the resulting Arrow Array is consistent with what user requested.

Demonstrate usage of foreign arrow function

ea3b81c

Including arrow language in the distribution by default. Added a basic example for creating an Arrow array.

hubertp added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Feb 23, 2024

Akirathan reviewed Feb 23, 2024

View reviewed changes

hubertp added 3 commits February 23, 2024 13:37

Missing Index_Out_Of_Bounds check

81677ff

Show that one can write to Arrow Array

b1c74e2

Missing file

bafbb1f

JaroslavTulach reviewed Feb 28, 2024

View reviewed changes

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso Outdated Show resolved Hide resolved

hubertp added 4 commits February 29, 2024 22:13

Rounding logic that ensure correct padding

95e1184

Arrow specification will enforce padding during serialization. This change is mostly copying from existing Java reference implementation. Also while the padding is now correct, allocation of separate `ByteDirect` buffers is not. TBC

Introduce Arrow_Array_Builder

43eb939

The builder accepts an Arrow array and allows for populating the array via the `set` method. Once the builder is `built` no more modifications are allowed.

Fix tests

6129fa6

hubertp marked this pull request as ready for review March 5, 2024 13:42

hubertp requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley and 4e6 as code owners March 5, 2024 13:42

hubertp commented Mar 5, 2024

View reviewed changes

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso Outdated Show resolved Hide resolved

jdunkerley approved these changes Mar 5, 2024

View reviewed changes

radeusgd reviewed Mar 5, 2024

View reviewed changes

...java/org/enso/interpreter/node/expression/builtin/interop/generic/WriteArrayElementNode.java Outdated Show resolved Hide resolved

radeusgd reviewed Mar 5, 2024

View reviewed changes

...src/main/java/org/enso/interpreter/node/expression/builtin/mutable/SetBuiltinVectorNode.java Outdated Show resolved Hide resolved

radeusgd reviewed Mar 5, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrowArrayBuilder.java Outdated Show resolved Hide resolved

radeusgd reviewed Mar 5, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrowArrayBuilder.java Outdated Show resolved Hide resolved

radeusgd reviewed Mar 5, 2024

View reviewed changes

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso Outdated Show resolved Hide resolved

radeusgd reviewed Mar 5, 2024

View reviewed changes

test/Base_Tests/src/Semantic/Arrow_Interop_Spec.enso Outdated Show resolved Hide resolved

JaroslavTulach requested changes Mar 6, 2024

View reviewed changes

hubertp added 2 commits March 6, 2024 11:13

PR comments

7e9d04a

Make builder generic (in name)

a8b3065

hubertp requested a review from JaroslavTulach March 6, 2024 11:27

hubertp commented Mar 6, 2024

View reviewed changes

nit

cf2a64f

radeusgd mentioned this pull request Mar 6, 2024

Implement Vector.build #9304

Closed

5 tasks

More specific error

db5c1db

enso-bot bot mentioned this pull request Mar 7, 2024

Prefix autoscoped constructors with .. #9275

Closed

JaroslavTulach requested changes Mar 7, 2024

View reviewed changes

hubertp added 3 commits March 7, 2024 23:34

Push builder into arrow language

bdf6646

Avoiding the introduction of various builtins by pushing builder pattern into allow language.

Merge branch 'develop' into wip/hubert/9118-arrow

71ad1fe

nit

e502751

enso-bot bot mentioned this pull request Mar 8, 2024

Trading shouldNotReachHere() for an abstract method #9312

Merged

2 tasks

JaroslavTulach approved these changes Mar 8, 2024

View reviewed changes

PR review

1e75bd6

hubertp requested a review from radeusgd March 8, 2024 11:39

nits

a34fe3e

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Mar 8, 2024

mergify bot merged commit f80dd9f into develop Mar 8, 2024
34 of 36 checks passed

mergify bot deleted the wip/hubert/9118-arrow branch March 8, 2024 15:20

enso-bot bot mentioned this pull request Mar 9, 2024

Provide Persistance for Persistance.Reference #9326

Merged

2 tasks

hubertp mentioned this pull request Mar 11, 2024

Arrow builder is not an Array #9358

Merged

2 tasks

hubertp mentioned this pull request Mar 15, 2024

Arrow language follow up #9118

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demonstrate usage of foreign arrow function #9150

Demonstrate usage of foreign arrow function #9150

hubertp commented Feb 23, 2024 •

edited

Loading

Akirathan Feb 23, 2024

JaroslavTulach left a comment

hubertp commented Mar 6, 2024

hubertp commented Mar 6, 2024

hubertp Mar 6, 2024

hubertp Mar 6, 2024

JaroslavTulach Mar 7, 2024

JaroslavTulach left a comment

JaroslavTulach Mar 7, 2024

JaroslavTulach Mar 7, 2024

JaroslavTulach Mar 7, 2024

JaroslavTulach Mar 7, 2024

JaroslavTulach Mar 7, 2024

JaroslavTulach Mar 7, 2024

JaroslavTulach Mar 7, 2024

JaroslavTulach commented Mar 7, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Mar 8, 2024


		package org.enso.interpreter.arrow.runtime;

		public class RoundingUtil {

Demonstrate usage of foreign arrow function #9150

Demonstrate usage of foreign arrow function #9150

Conversation

hubertp commented Feb 23, 2024 • edited Loading

Pull Request Description

Important notes

Checklist

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

hubertp commented Mar 6, 2024

hubertp commented Mar 6, 2024

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Mar 7, 2024 • edited Loading

Arrow Array is already Enso Array

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented Feb 23, 2024 •

edited

Loading

JaroslavTulach commented Mar 7, 2024 •

edited

Loading