Arrow builder is not an Array

[hubertp]

Pull Request Description

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.

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.

[radeusgd]

Shouldn't we make it non-array completely by returning false in hasArrayElements?

I think, reading the PR description, we should not match:

case arrow_builder of
    _ : Array -> "still an array"
    _ -> "not an array"

and as far as I remember, if we respond hasArrayElements=true, then it will match such a case (I could be wrong here, anyway it feels like it may be worth adding such a test just to understand what the behaviour is supposed to be there).

[hubertp]

Shouldn't we make it non-array completely by returning false in hasArrayElements?

I think, reading the PR description, we should not match:

case arrow_builder of
    _ : Array -> "still an array"
    _ -> "not an array"

and as far as I remember, if we respond hasArrayElements=true, then it will match such a case (I could be wrong here, anyway it feels like it may be worth adding such a test just to understand what the behaviour is supposed to be there).

Depends. I would also have to manually enable all the related methods, e.g. length. Catch-22. Or as @JaroslavTulach put it, cheating ;P

[radeusgd]

Shouldn't we make it non-array completely by returning false in hasArrayElements?
I think, reading the PR description, we should not match:

case arrow_builder of
    _ : Array -> "still an array"
    _ -> "not an array"

and as far as I remember, if we respond hasArrayElements=true, then it will match such a case (I could be wrong here, anyway it feels like it may be worth adding such a test just to understand what the behaviour is supposed to be there).

Depends. I would also have to manually enable all the related methods, e.g. length. Catch-22. Or as @JaroslavTulach put it, cheating ;P

What do you mean manually enable?

I don't see any Catch-22 scenario here. The builder is a separate type with its own API. Why would we pretend it is an array when we don't really use it that way anyway?

[JaroslavTulach]

Why would we pretend it is an array when we don't really use it that way anyway?

That's basically the same question I asked here. Why?

[jdunkerley]

though Jaroslav said org.graalvm.collections was going away?

[JaroslavTulach]

Great observation. Thank you James. The question is how Pair gets on classpath? Probably as a result of some transitive dependencies. We should update the dependencies of Arrow language to not use a dependency with such a huge transitive closure, but rather more direct one which doesn't contain org.graalvm.collections as transitive dependency.

I don't know off-hand how such change should look like, but it'd be a good change.

[hubertp]

Why would we pretend it is an array when we don't really use it that way anyway?

That's basically the same question I asked here. Why?

In that case I misunderstood the conversation during the meeting because I got the impression that all the complaints were about at.
Sure, I can make the change.

[JaroslavTulach]

This implementation (together with isArrayElementReadable) guarantees that builder.at 1 is going to yield an exception.

[JaroslavTulach]

Why would we pretend it is an array when we don't really use it that way anyway?

That's basically the same question I asked here. Why?

... I got the impression that all the complaints were about at.

I believe the complaints were about at, length and all other methods derived from those base two - e.g. map, fold and everything else provided by Array.enso.

I believe you effectively disabled at and all other methods that call at behind the scene. I believe that neither map nor fold can succeed. All these methods yield an exception as soon as they make a call to at. Maybe one can call builder.length, but that's probably the only method that can be called without yielding an exception.

Why would we pretend it is an array when we don't really use it that way anyway?

Given no method defined by Array.enso can succeed (except length), it makes little sense to pretend builder : Array. At least neither I nor Radek see a reason why builder should pretend it is an Array when 99% methods defined on Array yield an exception.

[JaroslavTulach]

Yes, this looks OK.