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.

Sign up for GitHub

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

Jump to bottom

GH-37234: [MATLAB] Create an abstract arrow.type.TemporalType class #37236

Merged
merged 4 commits into from
Aug 18, 2023
Merged

GH-37234: [MATLAB] Create an abstract arrow.type.TemporalType class #37236

merged 4 commits into from
Aug 18, 2023

Conversation

sgilmore10
Copy link
Member

@sgilmore10 sgilmore10 commented Aug 17, 2023 •
edited
Loading

Rationale for this change

To simplify the implementation of Time32Type, Time64Type, Date32Type, and Date64Type, it would be helpful to create an abstract arrow.type.TemporalType class that the date and time types can inherit from.

As a first step, we can modify the implementation of TimestampType to inherit from TemporalType.

This mimics the class inheritance hierarchy from the C++ libraries.

What changes are included in this PR?

  1. Added a new MATLAB class called arrow.type.TemporalType. It inherits from arrow.type.FixedWidthType and defines one read-only property: TimeUnit.
  2. Modified the MATLAB class arrow.type.TimestampType to inherit from arrow.type.TemporalType instead of arrow.type.FixedWidthType.
  3. Renamed the C++ proxy::TimestampType methods timeUnit() and timeZone() to getTimeUnit() and getTimeZone().

Are these changes tested?

Yes. The existing tests in tTimetampType.m cover these changes.

Are there any user-facing changes?

No.

Future Directions

  1. Add arrow.type.Time32Type ([MATLAB] Add arrow.type.Time32Type class and arrow.time32 construction function #37231)
  2. Add arrow.type.Time64Type ([MATLAB] Add arrow.type.Time64Type class and arrow.time64 construction function #37232)
  3. Add arrow.type.Date32Type ([MATLAB] Add arrow.type.Date32Type class and arrow.date32 construction function #37229),
  4. Add arrow.type.Time64Type ([MATLAB] Add arrow.type.Date64Type class and arrow.date64 construction function #37230).
  5. Add arrow.array.Time32Array
  6. Add arrow.array.Time64Array
  7. Add arrow.array.Date32Array
  8. Add arrow.array.Date64Array

sgilmore10 and others added 4 commits August 17, 2023 16:11
@sgilmore10 @kevingurney
Add MATLAB class arrow.type.TemporalType
a5abe3b 
Co-authored-by: Kevin Gurney <kgurney@mathworks.com>
@sgilmore10
Modify TimestampType to inherit from TemporalType
8ba9c5d
@sgilmore10 @kevingurney
Rename the TimestampType proxy methods timeUnit() to getTimeUnit() an…
194c0ae 
…d timeZone() to getTimeZone()

Co-authored-by: Kevin Gurney <kgurney@mathworks.com>
@sgilmore10
Fix typo in TemporalType.m
fceae79
kou
kou approved these changes Aug 18, 2023
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Aug 18, 2023
kevingurney
kevingurney approved these changes Aug 18, 2023
View reviewed changes
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.

+1

@kevingurney kevingurney merged commit 5abdc6e into apache:main Aug 18, 2023
10 checks passed
@kevingurney kevingurney removed the awaiting merge Awaiting merge label Aug 18, 2023
@github-actions github-actions bot added the awaiting merge Awaiting merge label Aug 18, 2023
@sgilmore10 sgilmore10 deleted the GH-37234 branch August 18, 2023 13:58
This was referenced Aug 18, 2023
Closed
Merged
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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.

kevingurney pushed a commit that referenced this pull request Aug 21, 2023
@sgilmore10
GH-37251: [MATLAB] Make arrow.type.TemporalType a "tag" class (#37256)
06b0e4f 
### Rationale for this change

The original motivation for adding the super-class `arrow.type.TemporalType` in #37236 was to define a common implementation for extracting the `Unit` property from `TimestampType`, `Time32Type`, `Time64Type`, `Date32Type`, and `Date64Type`. However, this approach doesn't work because the `Unit` property on `Date32Type` and `Date64Type` is a `DateUnit`, while the `Unit` property on the other three types is a`TimeUnit`. As a result, we cannot define a shared method for extracting the `Unit` property in `TemporalType`. 

Instead, we plan on making `arrow.type.TemporalType` a "tag"-class (i.e. it has no properties or methods) so it can be used to group the "temporal" types together to ease authoring conditional logical in client code. In a future PR, we plan on adding functions like `arrow.type.isTemporal`, `arrow.type.isNumeric`, etc. so that clients don't need to query the class type information directly (i.e. call `isa(type, "arrow.type.TemporalType")`). 

```matlab
function doStuff(arrowArray)
  import arrow.*

  arrowType = arrowArray.Type;
  if type.isTemporal(arrowType)
      ...
  else if type.isNumeric(arrowType)
      ...
  else 
      ...
  end
end
```

### What changes are included in this PR?

1. Removed the `TimeUnit` property from `arrow.type.TemporalType`
2. Added `TimeUnit` back as a property on `arrow.type.TimestampType` and `arrow.type.Time32Type`

### Are these changes tested?

Yes, the existing tests cover these changes.

### Are there any user-facing changes?

No.

### Future Directions
1. #37232
2. #37229
3. #37230

* Closes: #37251

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
@sgilmore10 @kevingurney @loicalleyne
apacheGH-37234: [MATLAB] Create an abstract arrow.type.TemporalType…
f30a062 
… class (apache#37236)

### Rationale for this change

To simplify the implementation of `Time32Type`, `Time64Type`, `Date32Type`, and `Date64Type`, it would be helpful to create an abstract `arrow.type.TemporalType` class that the date and time types can inherit from.

As a first step, we can modify the implementation of `TimestampType` to inherit from `TemporalType`.

This mimics the class inheritance hierarchy from the C++ libraries.

### What changes are included in this PR?

1. Added a new MATLAB class called `arrow.type.TemporalType`. It inherits from `arrow.type.FixedWidthType` and defines one read-only property: `TimeUnit`.
2. Modified the MATLAB class `arrow.type.TimestampType` to inherit from `arrow.type.TemporalType` instead of `arrow.type.FixedWidthType`.
3. Renamed the  C++ `proxy::TimestampType` methods `timeUnit()` and `timeZone()` to `getTimeUnit()` and `getTimeZone()`.

### Are these changes tested?

Yes. The existing tests in `tTimetampType.m` cover these changes.

### Are there any user-facing changes?

No.

### Future Directions

1. Add `arrow.type.Time32Type` (apache#37231)
2. Add `arrow.type.Time64Type` (apache#37225)
3. Add `arrow.type.Date32Type` (apache#37229), 
4. Add `arrow.type.Time64Type` (apache#37230).
5. Add `arrow.array.Time32Array`
6. Add `arrow.array.Time64Array`
7. Add `arrow.array.Date32Array`
8. Add `arrow.array.Date64Array`
* Closes: apache#37234

Lead-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Co-authored-by: Kevin Gurney <kgurney@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
@sgilmore10 @loicalleyne
apacheGH-37251: [MATLAB] Make arrow.type.TemporalType a "tag" class (…
bb9e097 
…apache#37256)

### Rationale for this change

The original motivation for adding the super-class `arrow.type.TemporalType` in apache#37236 was to define a common implementation for extracting the `Unit` property from `TimestampType`, `Time32Type`, `Time64Type`, `Date32Type`, and `Date64Type`. However, this approach doesn't work because the `Unit` property on `Date32Type` and `Date64Type` is a `DateUnit`, while the `Unit` property on the other three types is a`TimeUnit`. As a result, we cannot define a shared method for extracting the `Unit` property in `TemporalType`. 

Instead, we plan on making `arrow.type.TemporalType` a "tag"-class (i.e. it has no properties or methods) so it can be used to group the "temporal" types together to ease authoring conditional logical in client code. In a future PR, we plan on adding functions like `arrow.type.isTemporal`, `arrow.type.isNumeric`, etc. so that clients don't need to query the class type information directly (i.e. call `isa(type, "arrow.type.TemporalType")`). 

```matlab
function doStuff(arrowArray)
  import arrow.*

  arrowType = arrowArray.Type;
  if type.isTemporal(arrowType)
      ...
  else if type.isNumeric(arrowType)
      ...
  else 
      ...
  end
end
```

### What changes are included in this PR?

1. Removed the `TimeUnit` property from `arrow.type.TemporalType`
2. Added `TimeUnit` back as a property on `arrow.type.TimestampType` and `arrow.type.Time32Type`

### Are these changes tested?

Yes, the existing tests cover these changes.

### Are there any user-facing changes?

No.

### Future Directions
1. apache#37232
2. apache#37229
3. apache#37230

* Closes: apache#37251

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
Reviewers

@kou kou kou approved these changes

@kevingurney kevingurney kevingurney approved these changes

Assignees
No one assigned
Labels
awaiting merge Awaiting merge Component: MATLAB
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MATLAB] Create an abstract arrow.type.TemporalType class
3 participants
@sgilmore10 @kou @kevingurney