Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-37825: [MATLAB] Improve arrow.type.Field display #37826

Merged
merged 4 commits into from
Sep 25, 2023

Conversation

sgilmore10
Copy link
Member

@sgilmore10 sgilmore10 commented Sep 21, 2023

Rationale for this change

We should improve the display of arrow.type.Field, which currently looks like this:

>> 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:

>> 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.

@sgilmore10 sgilmore10 changed the title GH-37825: [MATLAB] GH-37825: [MATLAB] Improve arrow.type.Field display Sep 21, 2023
Copy link
Member

@kevingurney kevingurney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you for the PR!

@kevingurney
Copy link
Member

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Sep 25, 2023
@kevingurney kevingurney merged commit 1ae2436 into apache:main Sep 25, 2023
13 checks passed
@kevingurney kevingurney deleted the GH-37825 branch September 25, 2023 15:56
@kevingurney kevingurney removed the awaiting merge Awaiting merge label Sep 25, 2023
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 1ae2436.

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

@jgehrcke
Copy link
Contributor

indicating a performance regression:

I looked into this. Does not seem to be an actual regression; also see conbench/conbench#594 (comment)

etseidl pushed a commit to etseidl/arrow that referenced this pull request Sep 28, 2023
…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>
kevingurney pushed a commit that referenced this pull request Sep 29, 2023
### Rationale for this change

We would like to change how `arrow.tabular.Schema`s are displayed in the Command Window. Below is the current display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>>s = arrow.schema([field1 field2])

s = 

A: time32[s]
B: bool
```

This display is not very MATLAB-like.

### What changes are included in this PR?

1. Updated the display of `arrow.tabular.Schema`. Below is the new display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>> s = arrow.schema([field1 field2])

s = 

  Arrow Schema with 2 fields:

    A: Time32 | B: Boolean
```

When MATLAB is opened in desktop mode, `Schema`, `Time32`, and `Boolean` are hyperlinks users can click on to view the help text for the different class types. 

### Are these changes tested?

Yes. Added three new test cases to `tSchema.m`: 
1. `TestDisplaySchemaZeroFields`
2. `TestDisplaySchemaOneField`
3. `TestDisplaySchemaMultipleFields`

### Are there any user-facing changes?

Yes. `arrow.tabular.Schema`s will be displayed differently in the Command Window.

### Notes

Once #37826 is merged, I will rebase my changes and mark this PR as ready for review.
* Closes: #37835

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…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>
JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…he#37836)

### Rationale for this change

We would like to change how `arrow.tabular.Schema`s are displayed in the Command Window. Below is the current display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>>s = arrow.schema([field1 field2])

s = 

A: time32[s]
B: bool
```

This display is not very MATLAB-like.

### What changes are included in this PR?

1. Updated the display of `arrow.tabular.Schema`. Below is the new display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>> s = arrow.schema([field1 field2])

s = 

  Arrow Schema with 2 fields:

    A: Time32 | B: Boolean
```

When MATLAB is opened in desktop mode, `Schema`, `Time32`, and `Boolean` are hyperlinks users can click on to view the help text for the different class types. 

### Are these changes tested?

Yes. Added three new test cases to `tSchema.m`: 
1. `TestDisplaySchemaZeroFields`
2. `TestDisplaySchemaOneField`
3. `TestDisplaySchemaMultipleFields`

### Are there any user-facing changes?

Yes. `arrow.tabular.Schema`s will be displayed differently in the Command Window.

### Notes

Once apache#37826 is merged, I will rebase my changes and mark this PR as ready for review.
* Closes: apache#37835

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…he#37836)

### Rationale for this change

We would like to change how `arrow.tabular.Schema`s are displayed in the Command Window. Below is the current display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>>s = arrow.schema([field1 field2])

s = 

A: time32[s]
B: bool
```

This display is not very MATLAB-like.

### What changes are included in this PR?

1. Updated the display of `arrow.tabular.Schema`. Below is the new display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>> s = arrow.schema([field1 field2])

s = 

  Arrow Schema with 2 fields:

    A: Time32 | B: Boolean
```

When MATLAB is opened in desktop mode, `Schema`, `Time32`, and `Boolean` are hyperlinks users can click on to view the help text for the different class types. 

### Are these changes tested?

Yes. Added three new test cases to `tSchema.m`: 
1. `TestDisplaySchemaZeroFields`
2. `TestDisplaySchemaOneField`
3. `TestDisplaySchemaMultipleFields`

### Are there any user-facing changes?

Yes. `arrow.tabular.Schema`s will be displayed differently in the Command Window.

### Notes

Once apache#37826 is merged, I will rebase my changes and mark this PR as ready for review.
* Closes: apache#37835

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…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>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…he#37836)

### Rationale for this change

We would like to change how `arrow.tabular.Schema`s are displayed in the Command Window. Below is the current display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>>s = arrow.schema([field1 field2])

s = 

A: time32[s]
B: bool
```

This display is not very MATLAB-like.

### What changes are included in this PR?

1. Updated the display of `arrow.tabular.Schema`. Below is the new display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>> s = arrow.schema([field1 field2])

s = 

  Arrow Schema with 2 fields:

    A: Time32 | B: Boolean
```

When MATLAB is opened in desktop mode, `Schema`, `Time32`, and `Boolean` are hyperlinks users can click on to view the help text for the different class types. 

### Are these changes tested?

Yes. Added three new test cases to `tSchema.m`: 
1. `TestDisplaySchemaZeroFields`
2. `TestDisplaySchemaOneField`
3. `TestDisplaySchemaMultipleFields`

### Are there any user-facing changes?

Yes. `arrow.tabular.Schema`s will be displayed differently in the Command Window.

### Notes

Once apache#37826 is merged, I will rebase my changes and mark this PR as ready for review.
* Closes: apache#37835

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…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>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…he#37836)

### Rationale for this change

We would like to change how `arrow.tabular.Schema`s are displayed in the Command Window. Below is the current display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>>s = arrow.schema([field1 field2])

s = 

A: time32[s]
B: bool
```

This display is not very MATLAB-like.

### What changes are included in this PR?

1. Updated the display of `arrow.tabular.Schema`. Below is the new display:

```matlab
>> field1 =  arrow.field("A", arrow.time32());
>> field2 = arrow.field("B", arrow.boolean());
>> s = arrow.schema([field1 field2])

s = 

  Arrow Schema with 2 fields:

    A: Time32 | B: Boolean
```

When MATLAB is opened in desktop mode, `Schema`, `Time32`, and `Boolean` are hyperlinks users can click on to view the help text for the different class types. 

### Are these changes tested?

Yes. Added three new test cases to `tSchema.m`: 
1. `TestDisplaySchemaZeroFields`
2. `TestDisplaySchemaOneField`
3. `TestDisplaySchemaMultipleFields`

### Are there any user-facing changes?

Yes. `arrow.tabular.Schema`s will be displayed differently in the Command Window.

### Notes

Once apache#37826 is merged, I will rebase my changes and mark this PR as ready for review.
* Closes: apache#37835

Authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MATLAB] Improve arrow.type.Field display
3 participants