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 2023/09/06 18:14:15 UTC
[GitHub] [arrow] kevingurney commented on a diff in pull request #37593: GH-37591: [MATLAB] Make `arrow.type.Type` inherit from `matlab.mixin.Heterogeneous`
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