apacheGH-37825: [MATLAB] Improve arrow.type.Field display (apache#3…

…7826) ### Rationale for this change We should improve the display of `arrow.type.Field`, which currently looks like this: ```matlab >> arrow.field("A", arrow.int32()) ans = A: int32 ``` This display isn't very "MATLAB-like". For instance, it doesn't display the object's class type. This display would be better: ```matlab >> arrow.field("A", arrow.int32()) ans = Field with properties: Name: "A" Type: [1x1 arrow.type.Int32Type] ``` ### What changes are included in this PR? 1. Added `getPropertyGroups` method to `Field`. This method is inherited from the superclass `matlab.mixin.CustomDisplay`. 2. Removed `displayScalarObject` method from `Field`. This method is also inherited from `matlab.mixin.CustomDisplay`. By implementing `getPropertyGroups`, we no longer need to override `displayScalarObject` and can use the default implementation of this method in `CustomDisplay`. 3. Removed `toString()` method from `Field`. This method was private, and only used by `displayScalarObject`. Since `displayScalarObject` has been removed, `toString()` can be deleted too. 4. Converted the helper test methods (`makeLinkString`, `makeDimensionString`, `verifyDisplay`) in `tTypeDisplay` into standalone functions. Test classes other than `tTypeDisplay.m` can now use these utilities as well. ### Are these changes tested? Yes. Added a `TestDisplay` unit test to `tField.m`. ### Are there any user-facing changes? Yes. `arrow.type.Field` objects are now displayed differently in the Command Window. ### Future Directions 1. Update the display of `arrow.tabular.Schema`. 2. Update the display of `arrow.array.Array`. 3. Update the display of `arrow.tabular.Table`. 4. Update the display of `arrow.tabular.RecordBatch`. * Closes: apache#37825 Authored-by: Sarah Gilmore <[email protected]> Signed-off-by: Kevin Gurney <[email protected]>
JerAguilon · Oct 23, 2023 · dd9f1c9 · dd9f1c9
1 parent 605b070
commit dd9f1c9
Show file tree

Hide file tree

Showing 8 changed files with 157 additions and 70 deletions.
diff --git a/matlab/src/cpp/arrow/matlab/type/proxy/field.cc b/matlab/src/cpp/arrow/matlab/type/proxy/field.cc
@@ -32,7 +32,6 @@ namespace arrow::matlab::type::proxy {
     Field::Field(std::shared_ptr<arrow::Field> field) : field{std::move(field)} {
         REGISTER_METHOD(Field, getName);
         REGISTER_METHOD(Field, getType);
-        REGISTER_METHOD(Field, toString);
     }
 
     std::shared_ptr<arrow::Field> Field::unwrap() {
@@ -64,16 +63,6 @@ namespace arrow::matlab::type::proxy {
         context.outputs[0] = output;
     }
 
-    void Field::toString(libmexclass::proxy::method::Context& context) {
-        namespace mda = ::matlab::data;
-        mda::ArrayFactory factory;
-
-        const auto str_utf8 = field->ToString();
-        MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(const auto str_utf16, arrow::util::UTF8StringToUTF16(str_utf8), context, error::UNICODE_CONVERSION_ERROR_ID);
-        auto str_mda = factory.createScalar(str_utf16);
-        context.outputs[0] = str_mda;
-    }
-
     libmexclass::proxy::MakeResult Field::make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
         namespace mda = ::matlab::data;
         using FieldProxy = arrow::matlab::type::proxy::Field;

diff --git a/matlab/src/cpp/arrow/matlab/type/proxy/field.h b/matlab/src/cpp/arrow/matlab/type/proxy/field.h
@@ -36,7 +36,6 @@ class Field : public libmexclass::proxy::Proxy {
     protected:
         void getName(libmexclass::proxy::method::Context& context);
         void getType(libmexclass::proxy::method::Context& context);
-        void toString(libmexclass::proxy::method::Context& context);
 
         std::shared_ptr<arrow::Field> field;
 };

diff --git a/matlab/src/matlab/+arrow/+internal/+test/+display/makeDimensionString.m b/matlab/src/matlab/+arrow/+internal/+test/+display/makeDimensionString.m
@@ -0,0 +1,22 @@
+%MAKEDIMENSIONSTRING Utility function for creating a string representation
+%of dimensions.
+
+% 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.
+
+function dimensionString = makeDimensionString(arraySize)
+    dimensionString = string(arraySize);
+    dimensionString = join(dimensionString, char(215));
+end
diff --git a/matlab/src/matlab/+arrow/+internal/+test/+display/makeLinkString.m b/matlab/src/matlab/+arrow/+internal/+test/+display/makeLinkString.m
@@ -0,0 +1,36 @@
+%MAKELINKSTRING Utility function for creating hyperlinks.
+
+% 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.
+
+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
diff --git a/matlab/src/matlab/+arrow/+internal/+test/+display/verify.m b/matlab/src/matlab/+arrow/+internal/+test/+display/verify.m
@@ -0,0 +1,32 @@
+%VERIFY Utility function used to verify object display.
+
+% 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.
+
+function verify(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
diff --git a/matlab/src/matlab/+arrow/+type/Field.m b/matlab/src/matlab/+arrow/+type/Field.m
@@ -91,16 +91,10 @@
         end
     end
 
-    methods (Access = private)
-        function str = toString(obj)
-            str = obj.Proxy.toString();
-        end
-    end
-
     methods (Access=protected)
-        function displayScalarObject(obj)
-            disp(obj.toString());
+        function groups = getPropertyGroups(~)
+            targets = ["Name", "Type"];
+            groups = matlab.mixin.util.PropertyGroup(targets);
         end
     end
-
 end
diff --git a/matlab/test/arrow/type/tField.m b/matlab/test/arrow/type/tField.m
@@ -231,5 +231,33 @@ function TestIsEqualNonScalarFalse(testCase)
             % Compare arrow.type.Field array and a string array
             testCase.verifyFalse(isequal(f1, strings(size(f1))));
         end
+
+        function TestDisplay(testCase)
+            % Verify the display of Field objects.
+            %
+            % Example:
+            %
+            %  Field with properties:
+            %
+            %        Name: FieldA
+            %        Type: [1x2 arrow.type.Int32Type]
+
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+            import arrow.internal.test.display.makeDimensionString
+
+            field = arrow.field("B", arrow.timestamp(TimeZone="America/Anchorage")); %#ok<NASGU>
+            classnameLink = makeLinkString(FullClassName="arrow.type.Field", ClassName="Field", BoldFont=true);
+            header = "  " + classnameLink + " with properties:" + newline;
+            body = strjust(pad(["Name:"; "Type:"]));
+            dimensionString = makeDimensionString([1 1]);
+            fieldString = compose("[%s %s]", dimensionString, "arrow.type.TimestampType");
+            body = body + " " + ["""B"""; fieldString];
+            body = "    " + body;
+            footer = string(newline);
+            expectedDisplay = char(strjoin([header body' footer], newline));
+            actualDisplay = evalc('disp(field)');
+            verify(testCase, actualDisplay, expectedDisplay);
+        end
     end
 end
diff --git a/matlab/test/arrow/type/tTypeDisplay.m b/matlab/test/arrow/type/tTypeDisplay.m
@@ -50,6 +50,10 @@ function EmptyTypeDisplay(testCase)
             %
             %    ID
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+            import arrow.internal.test.display.makeDimensionString
+
             type = arrow.type.Type.empty(0, 1);
             typeLink = makeLinkString(FullClassName="arrow.type.Type", ClassName="Type", BoldFont=true);
             dimensionString = makeDimensionString(size(type));
@@ -59,7 +63,7 @@ function EmptyTypeDisplay(testCase)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(type)');
-            testCase.verifyDisplay(actualDisplay, expectedDisplay);
+            verify(testCase, actualDisplay, expectedDisplay);
         end
 
         function NonScalarArrayDifferentTypes(testCase)
@@ -71,6 +75,10 @@ function NonScalarArrayDifferentTypes(testCase)
             %
             %    ID
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+            import arrow.internal.test.display.makeDimensionString
+
             float32Type = arrow.float32();
             timestampType = arrow.timestamp();
             typeArray = [float32Type timestampType];
@@ -88,7 +96,7 @@ function NonScalarArrayDifferentTypes(testCase)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(typeArray)');
-            testCase.verifyDisplay(actualDisplay, expectedDisplay);
+            verify(testCase, actualDisplay, expectedDisplay);
         end
 
         function NonScalarArraySameTypes(testCase)
@@ -102,6 +110,10 @@ function NonScalarArraySameTypes(testCase)
             %    TimeUnit
             %    TimeZone 
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+            import arrow.internal.test.display.makeDimensionString
+
             timestampType1 = arrow.timestamp(TimeZone="Pacific/Fiji");
             timestampType2 = arrow.timestamp(TimeUnit="Second");
             typeArray = [timestampType1 timestampType2];
@@ -114,7 +126,7 @@ function NonScalarArraySameTypes(testCase)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(typeArray)');
-            testCase.verifyDisplay(actualDisplay, expectedDisplay);
+            verify(testCase, actualDisplay, expectedDisplay);
         end
 
         function TestTypeDisplaysOnlyID(testCase, TypeDisplaysOnlyID)
@@ -127,6 +139,9 @@ function TestTypeDisplaysOnlyID(testCase, TypeDisplaysOnlyID)
             %
             %          ID: Boolean
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+
             type = TypeDisplaysOnlyID;
             fullClassName = string(class(type));
             className = reverse(extractBefore(reverse(fullClassName), "."));
@@ -136,7 +151,7 @@ function TestTypeDisplaysOnlyID(testCase, TypeDisplaysOnlyID)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(type)');
-            testCase.verifyDisplay(actualDisplay, expectedDisplay);
+            verify(testCase, actualDisplay, expectedDisplay);
         end
 
         function TestTimeType(testCase, TimeType)
@@ -149,6 +164,9 @@ function TestTimeType(testCase, TimeType)
             %          ID: Time32
             %    TimeUnit: Second
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+
             type = TimeType;
             fullClassName = string(class(type));
             className = reverse(extractBefore(reverse(fullClassName), "."));
@@ -161,7 +179,7 @@ function TestTimeType(testCase, TimeType)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(type)');
-            testCase.verifyEqual(actualDisplay, expectedDisplay);
+            verify(testCase, actualDisplay, expectedDisplay);
         end
 
         function TestDateType(testCase, DateType)
@@ -174,6 +192,9 @@ function TestDateType(testCase, DateType)
             %          ID: Date32
             %    DateUnit: Day
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+
             type = DateType;
             fullClassName = string(class(type));
             className = reverse(extractBefore(reverse(fullClassName), "."));
@@ -186,7 +207,7 @@ function TestDateType(testCase, DateType)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(type)');
-            testCase.verifyEqual(actualDisplay, expectedDisplay);
+            verify(testCase, actualDisplay, expectedDisplay);
         end
 
         function TimestampTypeDisplay(testCase)
@@ -200,6 +221,9 @@ function TimestampTypeDisplay(testCase)
             %    TimeUnit: Second
             %    TimeZone: "America/Anchorage"
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+
             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;
@@ -209,7 +233,7 @@ function TimestampTypeDisplay(testCase)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(type)');
-            testCase.verifyEqual(actualDisplay, expectedDisplay);
+            verify(testCase, actualDisplay, expectedDisplay);
         end
 
         function StructTypeDisplay(testCase)
@@ -222,6 +246,10 @@ function StructTypeDisplay(testCase)
             %          ID: Struct
             %      Fields: [1x2 arrow.type.Field]
 
+            import arrow.internal.test.display.verify
+            import arrow.internal.test.display.makeLinkString
+            import arrow.internal.test.display.makeDimensionString
+
             fieldA = arrow.field("A", arrow.int32());
             fieldB = arrow.field("B", arrow.timestamp(TimeZone="America/Anchorage"));
             type = arrow.struct(fieldA, fieldB); %#ok<NASGU>
@@ -235,48 +263,7 @@ function StructTypeDisplay(testCase)
             footer = string(newline);
             expectedDisplay = char(strjoin([header body' footer], newline));
             actualDisplay = evalc('disp(type)');
-            testCase.verifyDisplay(actualDisplay, expectedDisplay);
+            verify(testCase, 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 dimensionString = makeDimensionString(arraySize)
-    dimensionString = string(arraySize);
-    dimensionString = join(dimensionString, char(215));
-end