Skip to content

Commit

Permalink
apacheGH-37825: [MATLAB] Improve arrow.type.Field display (apache#3…
Browse files Browse the repository at this point in the history
…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 <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
  • Loading branch information
sgilmore10 authored and loicalleyne committed Nov 13, 2023
1 parent 9bb7d7d commit ddd965e
Show file tree
Hide file tree
Showing 8 changed files with 157 additions and 70 deletions.
11 changes: 0 additions & 11 deletions matlab/src/cpp/arrow/matlab/type/proxy/field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion matlab/src/cpp/arrow/matlab/type/proxy/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
Original file line number Diff line number Diff line change
@@ -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
36 changes: 36 additions & 0 deletions matlab/src/matlab/+arrow/+internal/+test/+display/makeLinkString.m
Original file line number Diff line number Diff line change
@@ -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
32 changes: 32 additions & 0 deletions matlab/src/matlab/+arrow/+internal/+test/+display/verify.m
Original file line number Diff line number Diff line change
@@ -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
12 changes: 3 additions & 9 deletions matlab/src/matlab/+arrow/+type/Field.m
Original file line number Diff line number Diff line change
Expand Up @@ -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
28 changes: 28 additions & 0 deletions matlab/test/arrow/type/tField.m
Original file line number Diff line number Diff line change
Expand Up @@ -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
85 changes: 36 additions & 49 deletions matlab/test/arrow/type/tTypeDisplay.m
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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)
Expand All @@ -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];
Expand All @@ -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)
Expand All @@ -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];
Expand All @@ -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)
Expand All @@ -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), "."));
Expand All @@ -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)
Expand All @@ -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), "."));
Expand All @@ -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)
Expand All @@ -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), "."));
Expand All @@ -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)
Expand All @@ -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;
Expand All @@ -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)
Expand All @@ -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>
Expand All @@ -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

0 comments on commit ddd965e

Please sign in to comment.