You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kevingurney (via GitHub)" <gi...@apache.org> on 2025/01/15 21:15:52 UTC

[PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

kevingurney opened a new pull request, #45274:
URL: https://github.com/apache/arrow/pull/45274

   To enable more workflows using the IPC Stream format in the MATLAB interface, this pull request adds the ability to construct a `RecordBatchStreamReader` from a MATLAB `uint8` array.
   
   This is helpful, for example, to enable Arrow-over-HTTP workflows in conjunction with the [MATLAB `webread` function](https://www.mathworks.com/help/matlab/ref/webread.html) (which can return a `uint8` array from an HTTP request).
   
   This is a followup issue to #44923.
   
   ### Rationale for this change
   
   <!--
    Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
    Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.  
   -->
   
   ### What changes are included in this PR?
   
   <!--
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   ### Are these changes tested?
   
   <!--
   We typically require tests for all PRs in order to:
   1. Prevent the code from being accidentally broken by subsequent changes
   2. Serve as another way to document the expected behavior of the code
   
   If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
   -->
   
   ### Are there any user-facing changes?
   
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!--
   If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
   -->
   <!-- **This PR includes breaking changes to public APIs.** -->
   
   <!--
   Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
   -->
   <!-- **This PR contains a "Critical Fix".** -->


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #45274:
URL: https://github.com/apache/arrow/pull/45274#discussion_r1920805520


##########
matlab/src/matlab/+arrow/+io/+ipc/RecordBatchStreamReader.m:
##########
@@ -26,14 +26,34 @@
         Schema
     end
 
-    methods
-        function obj = RecordBatchStreamReader(filename)

Review Comment:
   Thanks for the review, @kou!
   
   > FYI: If MATLAB has a keyword argument feature, we can consider another API design
   
   This is a very interesting point. MATLAB does in fact support name-value pairs - which *can* be used in this way. 
   
   However, there is limited design precedent for functions included with MATLAB which:
   
   1. Have zero required positional arguments *AND*
   2. Support name-value pairs that are all mutually exclusive of one another (i.e. you shouldn't use `Filename` and `Bytes` together)
   
   There are a few instances of this in MATLAB, but typically, MATLAB functions use name-value pairs for optional arguments in conjunction with required positional arguments.
   
   Despite the limited design precedent, this is still an interesting point, and this is certainly something we can think more about and potentially explore in the future.
   
   For the time being, it sounds like having two separate "construction" functions is fairly reasonable, so I think we can proceed with that design.
   
   Thanks again for your thoughtful feedback!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45274:
URL: https://github.com/apache/arrow/pull/45274#discussion_r1917510095


##########
matlab/src/matlab/+arrow/+io/+ipc/RecordBatchStreamReader.m:
##########
@@ -26,14 +26,34 @@
         Schema
     end
 
-    methods
-        function obj = RecordBatchStreamReader(filename)

Review Comment:
   Can we dispatch based on argument automatically instead of providing 2 constructors?
   
   For example:
   
   ```matlab
   function obj = RecordBatchStreamReader(filename_or_bytes)
       if filename_or_bytes.is_a(string)
           filename = filename_or_bytes
           % ...
       else
           bytes = filename_or_bytes
           % ...
       end
   end
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45274:
URL: https://github.com/apache/arrow/pull/45274#discussion_r1919472312


##########
matlab/src/matlab/+arrow/+io/+ipc/RecordBatchStreamReader.m:
##########
@@ -26,14 +26,34 @@
         Schema
     end
 
-    methods
-        function obj = RecordBatchStreamReader(filename)

Review Comment:
   Thanks for detailed explanation for this API design. My idea isn't a strong opinion. So this design is OK to me.
   
   FYI: If MATLAB has a keyword argument feature, we can consider another API design with it. e.g. `RecordBatchStreamReader(filename: path)` and `RecordBatchStreamReader(bytes: data)`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #45274:
URL: https://github.com/apache/arrow/pull/45274#discussion_r1919044845


##########
matlab/src/matlab/+arrow/+io/+ipc/RecordBatchStreamReader.m:
##########
@@ -26,14 +26,34 @@
         Schema
     end
 
-    methods
-        function obj = RecordBatchStreamReader(filename)

Review Comment:
   That's a good question. We could certainly do that if we wanted to and initially were leaning in this direction.
   
   However, there are a few reasons why we decided to go with the current approach:
   
   1. The majority of MATLAB classes that wrap a `libmexclass.proxy.Proxy` object have a single argument constructor that expects a `Proxy` instance, so separating out the "construction" functions this way allows `RecordBatchStreamReader` to follow an existing pattern that is used elsewhere in the MATLAB interface.
   
   2. If we end up wanting to support many different ways of constructing a particular Arrow type, then having a single constructor for that type won't always allow us to easily differentiate between similar input configurations. For example - if you wanted to support constructing a "CSV Reader" object from a filename *OR* an "in-memory buffer", it would be reasonable for both of these input configurations to be represented as scalar MATLAB `string` arrays. However, if this were the case - we wouldn't be able to distinguish one case from the other (i.e. they have identical function signatures).
   
   3. Technically, we could support both the `fromFile` and `fromBytes` functions *AND* dispatch to those functions in the constructor based on the input types. However, this is likely to add additional maintenance burden, and it is not clear if it will improve usability significantly for users.
   
   4. On a somewhat related note - we currently have `arrow.array(varargin)` - which is a "convenience" function - that can take in many different MATLAB array types and return a corresponding `arrow.array.Array` subclass. At the same time, we also have `arrow.array.[Type]Array.fromMATLAB(varargin)` - which achieves the same general goal, but does so in a slightly more explicit way for "developer-oriented" workflows. If we wanted to, we could always add a similar "convenience function" in the future like `arrow.ipcstream2table()`, `arrow.ipc2table()`, or `arrow.stream2table()` - which could, in theory, support multiple different input argument types and dispatch to the appropriate, corresponding static "construction" function under the hood (i.e. `fromBytes` or `fromFile`). However, this still wouldn't mitigate the limitation noted above about not being able to differentiate between two different "meanings" for the same input type (i.e. filename vs. in-memory buffer with a MATLAB `st
 ring` array).
   
   5. One more thing that might be worth thinking about is that - from a "discoverability" perspective - it seems like it might be easier for a new user to search for something like "Read Arrow IPC Stream format from bytes in MATLAB" and find a "fromBytes" function in the documentation, rather than having to explicitly check whether the `RecordBatchStreamReader` constructor supports the particular configuration that their workflow requires. In addition, it seems like this could potentially get more difficult over time if the number of distinct function signatures supported by the constructor increases.
   
   Hopefully, this reasoning makes some sense. A lot of these points are definitely debatable and most of them basically come down to "style". We can certainly see valid arguments against this design, as well. If anyone has strong opinions against this, we are more than happy to modify the design accordingly.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #45274:
URL: https://github.com/apache/arrow/pull/45274#discussion_r1919044845


##########
matlab/src/matlab/+arrow/+io/+ipc/RecordBatchStreamReader.m:
##########
@@ -26,14 +26,34 @@
         Schema
     end
 
-    methods
-        function obj = RecordBatchStreamReader(filename)

Review Comment:
   That's a good question. We could certainly do that if we wanted to and initially were leaning in this direction.
   
   However, there are a few reasons we we decided to go with the current approach:
   
   1. The majority of MATLAB classes that wrap a `libmexclass.proxy.Proxy` object have a single argument constructor that expects a `Proxy` instance, so separating out the "construction" functions this way allows `RecordBatchStreamReader` to follow an existing pattern that is used elsewhere in the MATLAB interface.
   
   2. If we end up wanting to support many different ways of constructing a particular Arrow type, then having a single constructor for that type won't always allow us to easily differentiate between similar input configurations. For example - if you wanted to support constructing a "CSV Reader" object from a filename *OR* an "in-memory buffer", it would be reasonable for both of these input configurations to be represented as scalar MATLAB `string` arrays. However, if this were the case - we wouldn't be able to distinguish one case from the other (i.e. they have identical function signatures).
   
   3. Technically, we could support both the `fromFile` and `fromBytes` functions *AND* dispatch to those functions in the constructor based on the input types. However, this is likely to add additional maintenance burden, and it is not clear if it will improve usability significantly for users.
   
   4. On a somewhat related note - we currently have `arrow.array(varargin)` - which is a "convenience" function - that can take in many different MATLAB array types and return a corresponding `arrow.array.Array` subclass. At the same time, we also have `arrow.array.[Type]Array.fromMATLAB(varargin)` - which achieves the same general goal, but does so in a slightly more explicit way for "developer-oriented" workflows. If we wanted to, we could always add a similar "convenience function" in the future like `arrow.ipcstream2table()`, `arrow.ipc2table()`, or `arrow.stream2table()` - which could, in theory, support multiple different input argument types and dispatch to the appropriate, corresponding static "construction" function under the hood (i.e. `fromBytes` or `fromFile`). However, this still wouldn't mitigate the limitation noted above about not being able to differentiate between two different "meanings" for the same input type (i.e. filename vs. in-memory buffer with a MATLAB `st
 ring` array).
   
   5. One more thing that might be worth thinking about is that - from a "discoverability" perspective - it seems like it might be easier for a new user to search for something like "Read Arrow IPC Stream format from bytes in MATLAB" and find a "fromBytes" function in the documentation, rather than having to explicitly check whether the `RecordBatchStreamReader` constructor supports the particular configuration that their workflow requires. In addition, it seems like this could potentially get more difficult over time if the number of distinct function signatures supported by the constructor increases.
   
   Hopefully, this reasoning makes some sense. A lot of these points are definitely debatable, and we can certainly see valid arguments against this design, as well. If anyone has strong opinions against this, we are happy to modify the design accordingly.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #45274:
URL: https://github.com/apache/arrow/pull/45274#issuecomment-2599527551

   After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 1fe27fead07a299325e363bb889559a49c09d6ab.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/35810396604) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #45274:
URL: https://github.com/apache/arrow/pull/45274#issuecomment-2599275923

   +1


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney merged PR #45274:
URL: https://github.com/apache/arrow/pull/45274


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45263: [MATLAB] Add ability to construct `RecordBatchStreamReader` from `uint8` array [arrow]

Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #45274:
URL: https://github.com/apache/arrow/pull/45274#discussion_r1919044845


##########
matlab/src/matlab/+arrow/+io/+ipc/RecordBatchStreamReader.m:
##########
@@ -26,14 +26,34 @@
         Schema
     end
 
-    methods
-        function obj = RecordBatchStreamReader(filename)

Review Comment:
   That's a good question. We could certainly do that if we wanted to and initially were leaning in this direction.
   
   However, there are a few reasons why we decided to go with the current approach:
   
   1. The majority of MATLAB classes that wrap a `libmexclass.proxy.Proxy` object have a single argument constructor that expects a `Proxy` instance, so separating out the "construction" functions this way allows `RecordBatchStreamReader` to follow an existing pattern that is used elsewhere in the MATLAB interface.
   
   2. If we end up wanting to support many different ways of constructing a particular Arrow type, then having a single constructor for that type won't always allow us to easily differentiate between similar input configurations. For example - if you wanted to support constructing a "CSV Reader" object from a filename *OR* an "in-memory buffer", it would be reasonable for both of these input configurations to be represented as scalar MATLAB `string` arrays. However, if this were the case - we wouldn't be able to distinguish one case from the other (i.e. they have identical function signatures).
   
   3. Technically, we could support both the `fromFile` and `fromBytes` functions *AND* dispatch to those functions in the constructor based on the input types. However, this is likely to add additional maintenance burden, and it is not clear if it will improve usability significantly for users.
   
   4. On a somewhat related note - we currently have `arrow.array(varargin)` - which is a "convenience" function - that can take in many different MATLAB array types and return a corresponding `arrow.array.Array` subclass. At the same time, we also have `arrow.array.[Type]Array.fromMATLAB(varargin)` - which achieves the same general goal, but does so in a slightly more explicit way for "developer-oriented" workflows. If we wanted to, we could always add a similar "convenience function" in the future like `arrow.ipcstream2table()`, `arrow.ipc2table()`, or `arrow.stream2table()` - which could, in theory, support multiple different input argument types and dispatch to the appropriate, corresponding static "construction" function under the hood (i.e. `fromBytes` or `fromFile`). However, this still wouldn't mitigate the limitation noted above about not being able to differentiate between two different "meanings" for the same input type (i.e. filename vs. in-memory buffer with a MATLAB `st
 ring` array).
   
   5. One more thing that might be worth thinking about is that - from a "discoverability" perspective - it seems like it might be easier for a new user to search for something like "Read Arrow IPC Stream format from bytes in MATLAB" and find a "fromBytes" function in the documentation, rather than having to explicitly check whether the `RecordBatchStreamReader` constructor supports the particular configuration that their workflow requires. In addition, it seems like this could potentially get more difficult over time if the number of distinct function signatures supported by the constructor increases.
   
   Hopefully, this reasoning makes some sense. A lot of these points are definitely debatable, and we can certainly see valid arguments against this design, as well. If anyone has strong opinions against this, we are happy to modify the design accordingly.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org