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-37251: [MATLAB] Make arrow.type.TemporalType a "tag" class #37256

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

sgilmore10
Copy link
Member

@sgilmore10 sgilmore10 commented Aug 18, 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 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

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. [MATLAB] Add arrow.type.Time64Type class and arrow.time64 construction function #37232
  2. [MATLAB] Add arrow.type.Date32Type class and arrow.date32 construction function #37229
  3. [MATLAB] Add arrow.type.Date64Type class and arrow.date64 construction function #37230

@github-actions
Copy link

⚠️ GitHub issue #37251 has been automatically assigned in GitHub to PR creator.

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

FYI: There is TimeType between Time{32,64}Type and TemporalType in C++:

arrow/cpp/src/arrow/type.h

Lines 1333 to 1381 in 6c660a5

/// Base type class for time data
class ARROW_EXPORT TimeType : public TemporalType, public ParametricType {
public:
TimeUnit::type unit() const { return unit_; }
protected:
TimeType(Type::type type_id, TimeUnit::type unit);
std::string ComputeFingerprint() const override;
TimeUnit::type unit_;
};
/// Concrete type class for 32-bit time data (as number of seconds or milliseconds
/// since midnight)
class ARROW_EXPORT Time32Type : public TimeType {
public:
static constexpr Type::type type_id = Type::TIME32;
using c_type = int32_t;
using PhysicalType = Int32Type;
static constexpr const char* type_name() { return "time32"; }
int bit_width() const override { return static_cast<int>(sizeof(c_type) * CHAR_BIT); }
explicit Time32Type(TimeUnit::type unit = TimeUnit::MILLI);
std::string ToString() const override;
std::string name() const override { return "time32"; }
};
/// Concrete type class for 64-bit time data (as number of microseconds or nanoseconds
/// since midnight)
class ARROW_EXPORT Time64Type : public TimeType {
public:
static constexpr Type::type type_id = Type::TIME64;
using c_type = int64_t;
using PhysicalType = Int64Type;
static constexpr const char* type_name() { return "time64"; }
int bit_width() const override { return static_cast<int>(sizeof(c_type) * CHAR_BIT); }
explicit Time64Type(TimeUnit::type unit = TimeUnit::NANO);
std::string ToString() const override;
std::string name() const override { return "time64"; }
};

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Aug 18, 2023
@sgilmore10
Copy link
Member Author

+1

FYI: There is TimeType between Time{32,64}Type and TemporalType in C++:

arrow/cpp/src/arrow/type.h

Lines 1333 to 1381 in 6c660a5

/// Base type class for time data
class ARROW_EXPORT TimeType : public TemporalType, public ParametricType {
public:
TimeUnit::type unit() const { return unit_; }
protected:
TimeType(Type::type type_id, TimeUnit::type unit);
std::string ComputeFingerprint() const override;
TimeUnit::type unit_;
};
/// Concrete type class for 32-bit time data (as number of seconds or milliseconds
/// since midnight)
class ARROW_EXPORT Time32Type : public TimeType {
public:
static constexpr Type::type type_id = Type::TIME32;
using c_type = int32_t;
using PhysicalType = Int32Type;
static constexpr const char* type_name() { return "time32"; }
int bit_width() const override { return static_cast<int>(sizeof(c_type) * CHAR_BIT); }
explicit Time32Type(TimeUnit::type unit = TimeUnit::MILLI);
std::string ToString() const override;
std::string name() const override { return "time32"; }
};
/// Concrete type class for 64-bit time data (as number of microseconds or nanoseconds
/// since midnight)
class ARROW_EXPORT Time64Type : public TimeType {
public:
static constexpr Type::type type_id = Type::TIME64;
using c_type = int64_t;
using PhysicalType = Int64Type;
static constexpr const char* type_name() { return "time64"; }
int bit_width() const override { return static_cast<int>(sizeof(c_type) * CHAR_BIT); }
explicit Time64Type(TimeUnit::type unit = TimeUnit::NANO);
std::string ToString() const override;
std::string name() const override { return "time64"; }
};

That's a good point. We should add an arrow.type.TimeType class to reduce code duplication between arrow.type.Time32Type and arrow.type.Time64Type. I've created an issue (#37262) capturing this enhancement.

@kevingurney
Copy link
Member

+1

@kevingurney kevingurney merged commit 06b0e4f into apache:main Aug 21, 2023
10 checks passed
@kevingurney kevingurney removed the awaiting merge Awaiting merge label Aug 21, 2023
@github-actions github-actions bot added the awaiting merge Awaiting merge label Aug 21, 2023
@sgilmore10 sgilmore10 deleted the GH-37251 branch August 21, 2023 18:12
@conbench-apache-arrow
Copy link

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

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.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request 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 this pull request may close these issues.

[MATLAB] Make arrow.type.TemporalType a "tag" class
3 participants