You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "sgilmore10 (via GitHub)" <gi...@apache.org> on 2023/09/06 16:25:58 UTC
[GitHub] [arrow] sgilmore10 opened a new pull request, #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
sgilmore10 opened a new pull request, #37593:
URL: https://github.com/apache/arrow/pull/37593
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
We should modify `arrow.type.Type` to inherit from `matlab.mixin.Heterogeneous` so that it's possible to create a nonscalar array containing different concrete subclasses of `arrow.type.Type`.
**Example**
```matlab
>> float32Type = arrow.float32();
>> timestampType = arrow.timestamp(TimeZone="Pacific/Fiji");
>> typeArray = [float32Type timestampType]
typeArray =
1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
ID
```
Currently, running the example above results in an error:
```matlab
>> float32Type = arrow.float32();
>> timestampType = arrow.timestamp(TimeZone="Pacific/Fiji");
>> typeArray = [float32Type timestampType]
Error using horzcat
The following error occurred converting from arrow.type.TimestampType to arrow.type.Float32Type:
Invalid argument at position 1. Value must be of type libmexclass.proxy.Proxy or be convertible to libmexclass.proxy.Proxy.
```
### What changes are included in this PR?
1. Updated `arrow.type.Type` to inherit from `matlab.mixin.Heterogeneous`. Previously, it only inherited from `matlab.mixin.CustomDisplay`.
2. Because `arrow.type.Type` is now `heterogeneous`, its subclasses cannot override the protected method `getPropertyGroups` - which is inherited from `matlab.mixin.CustomDisplay`. This method must be `Sealed` and defined on `Type` because only `Sealed` methods can be called on nonscalar `heterogeneous` arrays. To get around this limitation, I defined a new abstract template method called `getDisplayPropertyGroups`, which `getPropertyGroups` invokes if the array is either scalar or nonscalar and `homogeneous`. Refer to [this link](https://www.mathworks.com/help/matlab/matlab_oop/custom-display-for-heterogeneous-arrays.html) for more details.
**Displaying a nonscalar *heterogeneous* `arrow.type.Type`**
```matlab
>> float32Type = arrow.timestamp(TimeUnit="Second");
>> timestampType = arrow.timestamp(TimeZone="Pacific/Fiji");
>> typeArray = [float32Type timestampType]
typeArray =
1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
ID
```
**Displaying a nonscalar *homogeneous* `arrow.type.Type`**
```matlab
>> timestampType1 = arrow.timestamp(TimeUnit="Second");
>> timestampType2 = arrow.timestamp(TimeZone="Pacific/Fiji");
>> typeArray = [timestampType1 timestampType2]
typeArray =
1×2 TimestampType array with properties:
ID
TimeUnit
TimeZone
```
### Are these changes tested?
Yes
1. Added a new test class in `test/arrow/type` called `tDisplay.m`
2. Added display unit tests for all concrete subclasses of `arrow.type.Type` if they did not already exists.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
6. 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?
Yes. Users can now create nonscalar arrays with different concrete subclasses of `arrow.type.Type`.
<!--
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
[GitHub] [arrow] kevingurney commented on pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #37593:
URL: https://github.com/apache/arrow/pull/37593#issuecomment-1709062146
+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
[GitHub] [arrow] sgilmore10 commented on a diff in pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #37593:
URL: https://github.com/apache/arrow/pull/37593#discussion_r1317671571
##########
matlab/test/arrow/type/tDisplay.m:
##########
@@ -0,0 +1,133 @@
+%TDISPLAY Unit tests verifying the display of arrow.type.Type arrays.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tDisplay < matlab.unittest.TestCase
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1); %#ok<NASGU>
+ classnameLink = "<a href=""matlab:helpPopup arrow.type.Type"" style=""font-weight:bold"">Type</a>";
+ sizeString = "0" + char(215) + "1";
+ header = " " + sizeString + " " + classnameLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeDisplayLink(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeDisplayLink(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeDisplayLink(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ sizeString = makeSizeString(size(typeArray));
+
+ header = " " + sizeString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
Review Comment:
My bad. I'll fix that!
--
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
[GitHub] [arrow] kevingurney merged pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney merged PR #37593:
URL: https://github.com/apache/arrow/pull/37593
--
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
[GitHub] [arrow] sgilmore10 commented on pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on PR #37593:
URL: https://github.com/apache/arrow/pull/37593#issuecomment-1708950488
> Overall, this looks good to me!
>
> Just one high level comment - I think it would make sense to move all of the type-specific display tests you added into `tDisplay` for simplicity (this will also allow you to use your new helper method `makeDisplayLink` in these tests).
Good idea! I just did this.
--
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
[GitHub] [arrow] sgilmore10 commented on a diff in pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #37593:
URL: https://github.com/apache/arrow/pull/37593#discussion_r1317681713
##########
matlab/test/arrow/type/tDisplay.m:
##########
@@ -0,0 +1,133 @@
+%TDISPLAY Unit tests verifying the display of arrow.type.Type arrays.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tDisplay < matlab.unittest.TestCase
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1); %#ok<NASGU>
+ classnameLink = "<a href=""matlab:helpPopup arrow.type.Type"" style=""font-weight:bold"">Type</a>";
+ sizeString = "0" + char(215) + "1";
+ header = " " + sizeString + " " + classnameLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeDisplayLink(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeDisplayLink(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeDisplayLink(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ sizeString = makeSizeString(size(typeArray));
+
+ header = " " + sizeString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ timestampType1 = arrow.timestamp(TimeZone="Pacific/Fiji");
+ timestampType2 = arrow.timestamp(TimeUnit="Second");
+ typeArray = [timestampType1 timestampType2]';
+
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ sizeString = makeSizeString(size(typeArray));
+ header = " " + sizeString + " " + timestampLink + " array with properties:" + newline;
+ body = strjust(["ID"; "TimeUnit"; "TimeZone"], "left");
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+ end
+
+ methods
+ function verifyDisplay(testCase, actualDisplay, expectedDisplay)
+ % When the MATLAB GUI is running, '×' (char(215)) is used the
+ % delimiter between dimension values. However, when the GUI is
+ % not running, 'x' (char(120)) is used as the delimiter.
+ % To account for this discrepancy, check if actualDisplay
+ % contains char(215). If not, replace all instances of
+ % char(215) in expectedDisplay with char(120).
+
+ tf = contains(actualDisplay, char(215));
+ if ~tf
+ idx = strfind(expectedDisplay, char(215));
+ expectedDisplay(idx) = char(120);
+ end
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+ end
+end
+
+function link = makeDisplayLink(opts)
+ arguments
+ opts.FullClassName(1, 1) string
+ opts.ClassName(1, 1) string
+ opts.BoldFont(1, 1) logical
+ end
+
+ if opts.BoldFont
+ link = compose("<a href=""matlab:helpPopup %s"" style=""font-weight:bold"">%s</a>", ...
+ opts.FullClassName, opts.ClassName);
+ else
+ link = compose("<a href=""matlab:helpPopup %s"">%s</a>", opts.FullClassName, opts.ClassName);
+ end
+end
+
+function sizeString = makeSizeString(arraySize)
Review Comment:
Agreed. This name isn't great. I decided to rename `makeSizeString` to `makeDimensionString` and `makeDisplayLink` to `makeLinkString`.
--
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
[GitHub] [arrow] kevingurney commented on a diff in pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37593:
URL: https://github.com/apache/arrow/pull/37593#discussion_r1317653780
##########
matlab/test/arrow/type/tDisplay.m:
##########
@@ -0,0 +1,133 @@
+%TDISPLAY Unit tests verifying the display of arrow.type.Type arrays.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tDisplay < matlab.unittest.TestCase
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1); %#ok<NASGU>
+ classnameLink = "<a href=""matlab:helpPopup arrow.type.Type"" style=""font-weight:bold"">Type</a>";
+ sizeString = "0" + char(215) + "1";
+ header = " " + sizeString + " " + classnameLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeDisplayLink(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeDisplayLink(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeDisplayLink(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ sizeString = makeSizeString(size(typeArray));
+
+ header = " " + sizeString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
Review Comment:
I think the example here isn't quite accurate relative to the test code. Looks like a copy/paste mistake.
##########
matlab/test/arrow/type/tDisplay.m:
##########
@@ -0,0 +1,133 @@
+%TDISPLAY Unit tests verifying the display of arrow.type.Type arrays.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tDisplay < matlab.unittest.TestCase
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1); %#ok<NASGU>
+ classnameLink = "<a href=""matlab:helpPopup arrow.type.Type"" style=""font-weight:bold"">Type</a>";
+ sizeString = "0" + char(215) + "1";
+ header = " " + sizeString + " " + classnameLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeDisplayLink(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeDisplayLink(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeDisplayLink(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ sizeString = makeSizeString(size(typeArray));
+
+ header = " " + sizeString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ timestampType1 = arrow.timestamp(TimeZone="Pacific/Fiji");
+ timestampType2 = arrow.timestamp(TimeUnit="Second");
+ typeArray = [timestampType1 timestampType2]';
+
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ sizeString = makeSizeString(size(typeArray));
+ header = " " + sizeString + " " + timestampLink + " array with properties:" + newline;
+ body = strjust(["ID"; "TimeUnit"; "TimeZone"], "left");
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+ end
+
+ methods
+ function verifyDisplay(testCase, actualDisplay, expectedDisplay)
+ % When the MATLAB GUI is running, '×' (char(215)) is used the
+ % delimiter between dimension values. However, when the GUI is
+ % not running, 'x' (char(120)) is used as the delimiter.
+ % To account for this discrepancy, check if actualDisplay
+ % contains char(215). If not, replace all instances of
+ % char(215) in expectedDisplay with char(120).
+
+ tf = contains(actualDisplay, char(215));
+ if ~tf
+ idx = strfind(expectedDisplay, char(215));
+ expectedDisplay(idx) = char(120);
+ end
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+ end
+end
+
+function link = makeDisplayLink(opts)
Review Comment:
Could you add a comment clarifying when links will be bold in the display?
##########
matlab/src/matlab/+arrow/+type/Type.m:
##########
@@ -42,14 +43,57 @@
end
end
- methods (Access=protected)
- function propgrp = getPropertyGroups(~)
- proplist = {'ID'};
- propgrp = matlab.mixin.util.PropertyGroup(proplist);
+ methods(Access = protected)
+ groups = getDisplayPropertyGroups(obj)
+ end
+
+ methods (Sealed, Access = protected)
+ function header = getHeader(obj)
+ header = getHeader@matlab.mixin.CustomDisplay(obj);
+ end
+
+ function groups = getPropertyGroups(obj)
+ if isscalar(obj)
+ groups = getDisplayPropertyGroups(obj);
+ else
+ % Check if every type in the array has the same class type.
+ % If so, call getDisplayPropertyGroups() so that all
+ % properties assoicated with that class are displayed.
+ classnames = arrayfun(@(type) string(class(type)), obj);
+ if numel(unique(classnames)) == 1
+ groups = getDisplayPropertyGroups(obj(1));
+ else
+ % If the array is heterogeneous, just display ID, which
+ % is the only property shared by all concrete
+ % subclasses of arrow.type.Type.
+ proplist = "ID";
+ groups = matlab.mixin.util.PropertyGroup(proplist);
+ end
+ end
+ end
+
+ function footer = getFooter(obj)
+ footer = getFooter@matlab.mixin.CustomDisplay(obj);
+ end
+
+ function displayNonScalarObject(obj)
+ displayNonScalarObject@matlab.mixin.CustomDisplay(obj);
+ end
+
+ function displayScalarObject(obj)
+ displayScalarObject@matlab.mixin.CustomDisplay(obj)
+ end
+
+ function displayEmptyObject(obj)
+ displayEmptyObject@matlab.mixin.CustomDisplay(obj);
+ end
+
+ function displayScalarHandleToDeletedObject(obj)
+ displayScalarHandleToDeletedObject@matlab.mixin.CustomDisplay(obj);
end
end
- methods
+ methods(Sealed)
Review Comment:
Could you add a space between `methods` and `(Sealed`)?
##########
matlab/test/arrow/type/tDisplay.m:
##########
@@ -0,0 +1,133 @@
+%TDISPLAY Unit tests verifying the display of arrow.type.Type arrays.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tDisplay < matlab.unittest.TestCase
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1); %#ok<NASGU>
+ classnameLink = "<a href=""matlab:helpPopup arrow.type.Type"" style=""font-weight:bold"">Type</a>";
Review Comment:
I think you can use your new `makeDisplayLink` helper function here,
##########
matlab/test/arrow/type/tDisplay.m:
##########
@@ -0,0 +1,133 @@
+%TDISPLAY Unit tests verifying the display of arrow.type.Type arrays.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tDisplay < matlab.unittest.TestCase
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1); %#ok<NASGU>
+ classnameLink = "<a href=""matlab:helpPopup arrow.type.Type"" style=""font-weight:bold"">Type</a>";
+ sizeString = "0" + char(215) + "1";
+ header = " " + sizeString + " " + classnameLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeDisplayLink(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeDisplayLink(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeDisplayLink(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ sizeString = makeSizeString(size(typeArray));
+
+ header = " " + sizeString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ timestampType1 = arrow.timestamp(TimeZone="Pacific/Fiji");
+ timestampType2 = arrow.timestamp(TimeUnit="Second");
+ typeArray = [timestampType1 timestampType2]';
+
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ sizeString = makeSizeString(size(typeArray));
+ header = " " + sizeString + " " + timestampLink + " array with properties:" + newline;
+ body = strjust(["ID"; "TimeUnit"; "TimeZone"], "left");
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+ end
+
+ methods
+ function verifyDisplay(testCase, actualDisplay, expectedDisplay)
+ % When the MATLAB GUI is running, '×' (char(215)) is used the
+ % delimiter between dimension values. However, when the GUI is
+ % not running, 'x' (char(120)) is used as the delimiter.
+ % To account for this discrepancy, check if actualDisplay
+ % contains char(215). If not, replace all instances of
+ % char(215) in expectedDisplay with char(120).
+
+ tf = contains(actualDisplay, char(215));
+ if ~tf
+ idx = strfind(expectedDisplay, char(215));
+ expectedDisplay(idx) = char(120);
+ end
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+ end
+end
+
+function link = makeDisplayLink(opts)
+ arguments
+ opts.FullClassName(1, 1) string
+ opts.ClassName(1, 1) string
+ opts.BoldFont(1, 1) logical
+ end
+
+ if opts.BoldFont
+ link = compose("<a href=""matlab:helpPopup %s"" style=""font-weight:bold"">%s</a>", ...
+ opts.FullClassName, opts.ClassName);
+ else
+ link = compose("<a href=""matlab:helpPopup %s"">%s</a>", opts.FullClassName, opts.ClassName);
+ end
+end
+
+function sizeString = makeSizeString(arraySize)
Review Comment:
Maybe we should rename this to something like `makeDimensionDisplay`?
##########
matlab/test/arrow/type/tDisplay.m:
##########
@@ -0,0 +1,133 @@
+%TDISPLAY Unit tests verifying the display of arrow.type.Type arrays.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tDisplay < matlab.unittest.TestCase
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1); %#ok<NASGU>
+ classnameLink = "<a href=""matlab:helpPopup arrow.type.Type"" style=""font-weight:bold"">Type</a>";
+ sizeString = "0" + char(215) + "1";
+ header = " " + sizeString + " " + classnameLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeDisplayLink(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeDisplayLink(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeDisplayLink(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ sizeString = makeSizeString(size(typeArray));
+
+ header = " " + sizeString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ timestampType1 = arrow.timestamp(TimeZone="Pacific/Fiji");
+ timestampType2 = arrow.timestamp(TimeUnit="Second");
+ typeArray = [timestampType1 timestampType2]';
+
+ timestampLink = makeDisplayLink(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ sizeString = makeSizeString(size(typeArray));
+ header = " " + sizeString + " " + timestampLink + " array with properties:" + newline;
+ body = strjust(["ID"; "TimeUnit"; "TimeZone"], "left");
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+ end
+
+ methods
+ function verifyDisplay(testCase, actualDisplay, expectedDisplay)
+ % When the MATLAB GUI is running, '×' (char(215)) is used the
Review Comment:
"used the" -> "used as the"
--
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
[GitHub] [arrow] sgilmore10 commented on a diff in pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #37593:
URL: https://github.com/apache/arrow/pull/37593#discussion_r1317742233
##########
matlab/test/arrow/type/tTypeDisplay.m:
##########
@@ -0,0 +1,256 @@
+%TTYPEDISPLAY Unit tests verifying the display of all classes within the
+%arrow.type.Type class hierarchy.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tTypeDisplay < matlab.unittest.TestCase
+
+ properties(TestParameter)
+ TypeDisplaysOnlyID = {arrow.boolean(), ...
+ arrow.uint8(), ...
+ arrow.uint16(), ...
+ arrow.uint32(), ...
+ arrow.uint64(), ...
+ arrow.int8(), ...
+ arrow.int16(), ...
+ arrow.int32(), ...
+ arrow.int64(), ...
+ arrow.float32(), ...
+ arrow.float64(), ...
+ arrow.float64(), ...
+ arrow.string()}
+
+ TimeType = {arrow.time32(TimeUnit="Second"), ...
+ arrow.time64(TimeUnit="Nanosecond")};
+
+ DateType = {arrow.date32(), ...
+ arrow.date64()};
+ end
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1);
+ typeLink = makeLinkString(FullClassName="arrow.type.Type", ClassName="Type", BoldFont=true);
+ dimensionString = makeDimensionString(size(type));
+ header = " " + dimensionString + " " + typeLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeLinkString(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeLinkString(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeLinkString(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeLinkString(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ dimensionString = makeDimensionString(size(typeArray));
+
+ header = " " + dimensionString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 TimestampType array with properties:
+ %
+ % ID
+ % TimeUnit
+ % TimeZone
+
+ timestampType1 = arrow.timestamp(TimeZone="Pacific/Fiji");
+ timestampType2 = arrow.timestamp(TimeUnit="Second");
+ typeArray = [timestampType1 timestampType2];
+
+ timestampLink = makeLinkString(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ dimensionString = makeDimensionString(size(typeArray));
+ header = " " + dimensionString + " " + timestampLink + " array with properties:" + newline;
+ body = strjust(["ID"; "TimeUnit"; "TimeZone"], "left");
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function TestTypeDisplaysOnlyID(testCase, TypeDisplaysOnlyID)
+ % Verify the display of arrow.type.Type subclasses that only
+ % display the ID property.
+ %
+ % Example:
+ %
+ % BooleanType with properties:
+ %
+ % ID: Boolean
+
+ type = TypeDisplaysOnlyID;
+ fullClassName = string(class(type));
+ className = reverse(extractBefore(reverse(fullClassName), "."));
+ typeLink = makeLinkString(FullClassName=fullClassName, ClassName=className, BoldFont=true);
+ header = " " + typeLink + " with properties:" + newline;
+ body = " ID: " + string(type.ID);
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function TestTimeType(testCase, TimeType)
+ % Verify the display of TimeType objects.
+ %
+ % Example:
+ %
+ % Time32Type with properties:
+ %
+ % ID: Time32
+ % TimeUnit: Second
+
+ type = TimeType;
+ fullClassName = string(class(type));
+ className = reverse(extractBefore(reverse(fullClassName), "."));
+ typeLink = makeLinkString(FullClassName=fullClassName, ClassName=className, BoldFont=true);
+
+ header = " " + typeLink + " with properties:" + newline;
+ body = strjust(pad(["ID:"; "TimeUnit:"]));
+ body = body + " " + [string(type.ID); string(type.TimeUnit)];
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+
+ function TestDateType(testCase, DateType)
+ % Verify the display of DateType objects.
+ %
+ % Example:
+ %
+ % Date32Type with properties:
+ %
+ % ID: Date32
+ % DateUnit: Day
+
+ type = DateType;
+ fullClassName = string(class(type));
+ className = reverse(extractBefore(reverse(fullClassName), "."));
+ typeLink = makeLinkString(FullClassName=fullClassName, ClassName=className, BoldFont=true);
+
+ header = " " + typeLink + " with properties:" + newline;
+ body = strjust(pad(["ID:"; "DateUnit:"]));
+ body = body + " " + [string(type.ID); string(type.DateUnit)];
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+
+ function Display(testCase)
+ % Verify the display of TimestampType objects.
+ %
+ % Example:
+ %
+ % TimestampType with properties:
+ %
+ % ID: Timestamp
+ % TimeUnit: Second
+ % TimeZone: "America/Anchorage"
+
+ type = arrow.timestamp(TimeUnit="Second", TimeZone="America/Anchorage"); %#ok<NASGU>
+ classnameLink = makeLinkString(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ header = " " + classnameLink + " with properties:" + newline;
+ body = strjust(pad(["ID:"; "TimeUnit:"; "TimeZone:"]));
+ body = body + " " + ["Timestamp"; "Second"; """America/Anchorage"""];
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+ end
+
+ methods
+ function verifyDisplay(testCase, actualDisplay, expectedDisplay)
+ % When the MATLAB GUI is running, '×' (char(215)) is used as
+ % the delimiter between dimension values. However, when the
+ % GUI is not running, 'x' (char(120)) is used as the delimiter.
+ % To account for this discrepancy, check if actualDisplay
+ % contains char(215). If not, replace all instances of
+ % char(215) in expectedDisplay with char(120).
+
+ tf = contains(actualDisplay, char(215));
+ if ~tf
+ idx = strfind(expectedDisplay, char(215));
+ expectedDisplay(idx) = char(120);
+ end
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+ end
+end
+
+function link = makeLinkString(opts)
+ arguments
+ opts.FullClassName(1, 1) string
+ opts.ClassName(1, 1) string
+ % When displaying heterogeneous arrays, only the name of the
+ % closest shared anscestor class is displayed in bold. All other
+ % class names are not bolded.
+ opts.BoldFont(1, 1) logical
+ end
+
+ if opts.BoldFont
+ link = compose("<a href=""matlab:helpPopup %s"" style=""font-weight:bold"">%s</a>", ...
+ opts.FullClassName, opts.ClassName);
+ else
+ link = compose("<a href=""matlab:helpPopup %s"">%s</a>", opts.FullClassName, opts.ClassName);
+ end
+end
+
+function sizeString = makeDimensionString(arraySize)
Review Comment:
ah yeah. Will do.
--
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
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #37593:
URL: https://github.com/apache/arrow/pull/37593#issuecomment-1710798168
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit f7b6ddbe8787fe62f5bde5cf9e686380c60adc40.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/16597728720) has more details.
--
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
[GitHub] [arrow] kevingurney commented on a diff in pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #37593:
URL: https://github.com/apache/arrow/pull/37593#discussion_r1317739376
##########
matlab/test/arrow/type/tTypeDisplay.m:
##########
@@ -0,0 +1,256 @@
+%TTYPEDISPLAY Unit tests verifying the display of all classes within the
+%arrow.type.Type class hierarchy.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tTypeDisplay < matlab.unittest.TestCase
+
+ properties(TestParameter)
+ TypeDisplaysOnlyID = {arrow.boolean(), ...
+ arrow.uint8(), ...
+ arrow.uint16(), ...
+ arrow.uint32(), ...
+ arrow.uint64(), ...
+ arrow.int8(), ...
+ arrow.int16(), ...
+ arrow.int32(), ...
+ arrow.int64(), ...
+ arrow.float32(), ...
+ arrow.float64(), ...
+ arrow.float64(), ...
+ arrow.string()}
+
+ TimeType = {arrow.time32(TimeUnit="Second"), ...
+ arrow.time64(TimeUnit="Nanosecond")};
+
+ DateType = {arrow.date32(), ...
+ arrow.date64()};
+ end
+
+ methods (Test)
+ function EmptyTypeDisplay(testCase)
+ % Verify the display of an empty arrow.type.Type instance.
+ %
+ % Example:
+ %
+ % 0x1 Type array with properties:
+ %
+ % ID
+
+ type = arrow.type.Type.empty(0, 1);
+ typeLink = makeLinkString(FullClassName="arrow.type.Type", ClassName="Type", BoldFont=true);
+ dimensionString = makeDimensionString(size(type));
+ header = " " + dimensionString + " " + typeLink + " array with properties:" + newline;
+ body = strjust(pad("ID"));
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArrayDifferentTypes(testCase)
+ % Verify the display of a nonscalar, heterogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 heterogeneous FixedWidthType (Float32Type, TimestampType) array with properties:
+ %
+ % ID
+
+ float32Type = arrow.float32();
+ timestampType = arrow.timestamp();
+ typeArray = [float32Type timestampType];
+
+ heterogeneousLink = makeLinkString(FullClassName="matlab.mixin.Heterogeneous", ClassName="heterogeneous", BoldFont=false);
+ fixedWidthLink = makeLinkString(FullClassName="arrow.type.FixedWidthType", ClassName="FixedWidthType", BoldFont=true);
+ timestampLink = makeLinkString(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=false);
+ float32Link = makeLinkString(FullClassName="arrow.type.Float32Type", ClassName="Float32Type", BoldFont=false);
+
+ dimensionString = makeDimensionString(size(typeArray));
+
+ header = " " + dimensionString + " " + heterogeneousLink + " " + fixedWidthLink + ...
+ " (" + float32Link + ", " + timestampLink + ") array with properties:" + newline;
+ body = " " + "ID";
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function NonScalarArraySameTypes(testCase)
+ % Verify the display of a scalar, homogeneous arrow.type.Type array:
+ %
+ % Example:
+ %
+ % 1×2 TimestampType array with properties:
+ %
+ % ID
+ % TimeUnit
+ % TimeZone
+
+ timestampType1 = arrow.timestamp(TimeZone="Pacific/Fiji");
+ timestampType2 = arrow.timestamp(TimeUnit="Second");
+ typeArray = [timestampType1 timestampType2];
+
+ timestampLink = makeLinkString(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ dimensionString = makeDimensionString(size(typeArray));
+ header = " " + dimensionString + " " + timestampLink + " array with properties:" + newline;
+ body = strjust(["ID"; "TimeUnit"; "TimeZone"], "left");
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(typeArray)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function TestTypeDisplaysOnlyID(testCase, TypeDisplaysOnlyID)
+ % Verify the display of arrow.type.Type subclasses that only
+ % display the ID property.
+ %
+ % Example:
+ %
+ % BooleanType with properties:
+ %
+ % ID: Boolean
+
+ type = TypeDisplaysOnlyID;
+ fullClassName = string(class(type));
+ className = reverse(extractBefore(reverse(fullClassName), "."));
+ typeLink = makeLinkString(FullClassName=fullClassName, ClassName=className, BoldFont=true);
+ header = " " + typeLink + " with properties:" + newline;
+ body = " ID: " + string(type.ID);
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyDisplay(actualDisplay, expectedDisplay);
+ end
+
+ function TestTimeType(testCase, TimeType)
+ % Verify the display of TimeType objects.
+ %
+ % Example:
+ %
+ % Time32Type with properties:
+ %
+ % ID: Time32
+ % TimeUnit: Second
+
+ type = TimeType;
+ fullClassName = string(class(type));
+ className = reverse(extractBefore(reverse(fullClassName), "."));
+ typeLink = makeLinkString(FullClassName=fullClassName, ClassName=className, BoldFont=true);
+
+ header = " " + typeLink + " with properties:" + newline;
+ body = strjust(pad(["ID:"; "TimeUnit:"]));
+ body = body + " " + [string(type.ID); string(type.TimeUnit)];
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+
+ function TestDateType(testCase, DateType)
+ % Verify the display of DateType objects.
+ %
+ % Example:
+ %
+ % Date32Type with properties:
+ %
+ % ID: Date32
+ % DateUnit: Day
+
+ type = DateType;
+ fullClassName = string(class(type));
+ className = reverse(extractBefore(reverse(fullClassName), "."));
+ typeLink = makeLinkString(FullClassName=fullClassName, ClassName=className, BoldFont=true);
+
+ header = " " + typeLink + " with properties:" + newline;
+ body = strjust(pad(["ID:"; "DateUnit:"]));
+ body = body + " " + [string(type.ID); string(type.DateUnit)];
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+
+ function Display(testCase)
+ % Verify the display of TimestampType objects.
+ %
+ % Example:
+ %
+ % TimestampType with properties:
+ %
+ % ID: Timestamp
+ % TimeUnit: Second
+ % TimeZone: "America/Anchorage"
+
+ type = arrow.timestamp(TimeUnit="Second", TimeZone="America/Anchorage"); %#ok<NASGU>
+ classnameLink = makeLinkString(FullClassName="arrow.type.TimestampType", ClassName="TimestampType", BoldFont=true);
+ header = " " + classnameLink + " with properties:" + newline;
+ body = strjust(pad(["ID:"; "TimeUnit:"; "TimeZone:"]));
+ body = body + " " + ["Timestamp"; "Second"; """America/Anchorage"""];
+ body = " " + body;
+ footer = string(newline);
+ expectedDisplay = char(strjoin([header body' footer], newline));
+ actualDisplay = evalc('disp(type)');
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+ end
+
+ methods
+ function verifyDisplay(testCase, actualDisplay, expectedDisplay)
+ % When the MATLAB GUI is running, '×' (char(215)) is used as
+ % the delimiter between dimension values. However, when the
+ % GUI is not running, 'x' (char(120)) is used as the delimiter.
+ % To account for this discrepancy, check if actualDisplay
+ % contains char(215). If not, replace all instances of
+ % char(215) in expectedDisplay with char(120).
+
+ tf = contains(actualDisplay, char(215));
+ if ~tf
+ idx = strfind(expectedDisplay, char(215));
+ expectedDisplay(idx) = char(120);
+ end
+ testCase.verifyEqual(actualDisplay, expectedDisplay);
+ end
+ end
+end
+
+function link = makeLinkString(opts)
+ arguments
+ opts.FullClassName(1, 1) string
+ opts.ClassName(1, 1) string
+ % When displaying heterogeneous arrays, only the name of the
+ % closest shared anscestor class is displayed in bold. All other
+ % class names are not bolded.
+ opts.BoldFont(1, 1) logical
+ end
+
+ if opts.BoldFont
+ link = compose("<a href=""matlab:helpPopup %s"" style=""font-weight:bold"">%s</a>", ...
+ opts.FullClassName, opts.ClassName);
+ else
+ link = compose("<a href=""matlab:helpPopup %s"">%s</a>", opts.FullClassName, opts.ClassName);
+ end
+end
+
+function sizeString = makeDimensionString(arraySize)
Review Comment:
Sorry, I should have added this comment before. Could we rename the return value to `dimensionString`?
--
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