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

[MATLAB] Make arrow.type.TemporalType a "tag" class #37251

Closed
sgilmore10 opened this issue Aug 18, 2023 · 0 comments · Fixed by #37256
Closed

[MATLAB] Make arrow.type.TemporalType a "tag" class #37251

sgilmore10 opened this issue Aug 18, 2023 · 0 comments · Fixed by #37256

Comments

@sgilmore10
Copy link
Member

sgilmore10 commented Aug 18, 2023

Describe the enhancement requested

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 aTimeUnit. 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")).

function doStuff(arrowArray)
  import arrow.*

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

Component(s)

MATLAB

@sgilmore10 sgilmore10 changed the title [MATLAB] Remove arrow.type.TemporalType superclass temporarily [MATLAB] Make arrow.type.TemporalType a "tag" class Aug 18, 2023
kevingurney added a commit that referenced this issue Aug 18, 2023
…2` construction function (#37250)

### Rationale for this change

To support the addition of `arrow.array.Time32Array`, this pull request adds a new `arrow.type.Time32Type` class and associated `arrow.time32` construction function to the MATLAB interface.

### What changes are included in this PR?

1. New `arrow.type.Time32Type` class.
2. New `arrow.time32` construction function that returns an `arrow.type.Time32Type` instance.

**Example**

```matlab
>> type = arrow.time32(TimeUnit="Millisecond")

type = 

  Time32Type with properties:

          ID: Time32
    TimeUnit: Millisecond

>> class(type)

ans =

    'arrow.type.Time32Type'
```

### Are these changes tested?

Yes.

1. Added new tests for `Time32` type ID enumeration to `tID`.
2. Added a test class `tTimeUnit` for the new validation function `arrow.internal.validate.temporal.timeUnit`.
4. Added a new test class `tTime32Type`.

### Are there any user-facing changes?

Yes.

There are two new user-facing APIs:

1. `arrow.time32` construction function
2. `arrow.type.Time32Type` class

### Future Directions:

1. #37232
2. #37229
3.  #37230
4. #37251

### Notes

1. Thank you @ sgilmore10 for your help with this pull request!
* Closes: #37231

Lead-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
kevingurney pushed a commit that referenced this issue Aug 21, 2023
### 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>
@kevingurney kevingurney added this to the 14.0.0 milestone Aug 21, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
….time32` construction function (apache#37250)

### Rationale for this change

To support the addition of `arrow.array.Time32Array`, this pull request adds a new `arrow.type.Time32Type` class and associated `arrow.time32` construction function to the MATLAB interface.

### What changes are included in this PR?

1. New `arrow.type.Time32Type` class.
2. New `arrow.time32` construction function that returns an `arrow.type.Time32Type` instance.

**Example**

```matlab
>> type = arrow.time32(TimeUnit="Millisecond")

type = 

  Time32Type with properties:

          ID: Time32
    TimeUnit: Millisecond

>> class(type)

ans =

    'arrow.type.Time32Type'
```

### Are these changes tested?

Yes.

1. Added new tests for `Time32` type ID enumeration to `tID`.
2. Added a test class `tTimeUnit` for the new validation function `arrow.internal.validate.temporal.timeUnit`.
4. Added a new test class `tTime32Type`.

### Are there any user-facing changes?

Yes.

There are two new user-facing APIs:

1. `arrow.time32` construction function
2. `arrow.type.Time32Type` class

### Future Directions:

1. apache#37232
2. apache#37229
3.  apache#37230
4. apache#37251

### Notes

1. Thank you @ sgilmore10 for your help with this pull request!
* Closes: apache#37231

Lead-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-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 issue Nov 13, 2023
…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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants