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

[GLib] Allow to create time and timestamp arrays with a time zone #39702

Closed
esodan opened this issue Jan 19, 2024 · 3 comments · Fixed by #39717
Closed

[GLib] Allow to create time and timestamp arrays with a time zone #39702

esodan opened this issue Jan 19, 2024 · 3 comments · Fixed by #39717

Comments

@esodan
Copy link

esodan commented Jan 19, 2024

Describe the enhancement requested

Current API to create a GArrowTimestamp, requires a GArrowTimestampDataType.

An GArrowTimestampDataType can be created but only is possible to define its time units; there isn't an API to create one adding a time zone as in PyArrow.

So a new garrow_timestamp_data_type_new_with_timezone() method is required using the following signature:

GArrowTimestampDataType *
garrow_timestamp_data_type_new_with_timezone (GArrowTimeUnit unit, GTimeZone* tz);

This should help GLib users to create timestamps with a time zone.

Above will require to add a method to get the current GTimeZone:

GTimeZone *
garrow_timestamp_get_timezone (GArrowTimestampArray *array);

Above API should be the added too, to GArrowTime32Array and GArrowTime64Array arrays, so is easy to know the time zone the time is representing.

Component(s)

GLib

@kou
Copy link
Member

kou commented Jan 19, 2024

Timezone support for GArrowTimestampDataType makes sense but it's for GArrowTime{32,64}DataType doesn't make sense. Because GArrowTime{32,64}DataType is seconds/milliseoncs/microseconds/nanoseconds since midnight: https://arrow.apache.org/docs/cpp/api/datatype.html#_CPPv4N5arrow10Time32TypeE

@esodan
Copy link
Author

esodan commented Jan 20, 2024

Timezone support for GArrowTimestampDataType makes sense but it's for GArrowTime{32,64}DataType doesn't make sense. Because GArrowTime{32,64}DataType is seconds/milliseoncs/microseconds/nanoseconds since midnight: https://arrow.apache.org/docs/cpp/api/datatype.html#_CPPv4N5arrow10Time32TypeE

I see. While I'm not an expert on time issues, but Time Zone could affect Date{32|64}; is it?

@kou
Copy link
Member

kou commented Jan 20, 2024

Time zone doesn't affect Date{32,64}. Because they are the number of days since UNIX epoch: https://arrow.apache.org/docs/cpp/api/datatype.html#_CPPv4N5arrow10Date32TypeE

Time zone doesn't change the number of days.

kou added a commit to kou/arrow that referenced this issue Jan 20, 2024
…taType

Timestamp data type in Apache Arrow supports time zone but Apache
Arrow C GLib didn't support it. Timestamp data type has
"timezone-aware" mode and "timezone-naive" mode. If a timestamp data
type has a valid time zone information, it uses "timezone-aware"
mode. If a timestamp data type doesn't have a valid time zone
information, it uses "timezone-naive" mode. Apache Arrow C GLib should
support both of them.

This change adds a new `GTimeZone *time_zone` argument to
`garrow_timestamp_data_type_new()` instead of adding a new
`garrow_timestamp_data_type_new_time_zone()` function. This breaks
backward compatibility for Apache Arrow C GLib users. But this still
keeps backward compatibility for users of bindings such as Ruby and
Vala. Because the new `GTimeZone *time_zone` is nullable.

I tried to use the "adding a new
`garrow_timestamp_data_type_new_time_zone()` function" approach but
Vala didn't like it. Both of
`garrow_timestamp_data_type_new_time_zone()` (constructor) and
`garrow_timestamp_data_type_get_time_zone()` (instance method or
property reader) were mapped to
`GArrow.TimestampDataType.time_zone()`.

So I chose the "adding a new argument to
`garrow_timestamp_data_type_new()`" approach.
kou added a commit that referenced this issue Jan 22, 2024
…#39717)

### Rationale for this change

Timestamp data type in Apache Arrow supports time zone but Apache Arrow C GLib didn't support it. Timestamp data type has "timezone-aware" mode and "timezone-naive" mode. If a timestamp data type has a valid time zone information, it uses "timezone-aware" mode. If a timestamp data type doesn't have a valid time zone information, it uses "timezone-naive" mode. Apache Arrow C GLib should support both of them.

### What changes are included in this PR?

This change adds a new `GTimeZone *time_zone` argument to `garrow_timestamp_data_type_new()` instead of adding a new `garrow_timestamp_data_type_new_time_zone()` function. This breaks backward compatibility for Apache Arrow C GLib users. But this still keeps backward compatibility for users of bindings such as Ruby and Vala. Because the new `GTimeZone *time_zone` is nullable.

I tried to use the "adding a new
`garrow_timestamp_data_type_new_time_zone()` function" approach but Vala didn't like it. Both of
`garrow_timestamp_data_type_new_time_zone()` (constructor) and `garrow_timestamp_data_type_get_time_zone()` (instance method or property reader) were mapped to
`GArrow.TimestampDataType.time_zone()`.

So I chose the "adding a new argument to
`garrow_timestamp_data_type_new()`" approach.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**

* Closes: #39702

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 16.0.0 milestone Jan 22, 2024
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…taType (apache#39717)

### Rationale for this change

Timestamp data type in Apache Arrow supports time zone but Apache Arrow C GLib didn't support it. Timestamp data type has "timezone-aware" mode and "timezone-naive" mode. If a timestamp data type has a valid time zone information, it uses "timezone-aware" mode. If a timestamp data type doesn't have a valid time zone information, it uses "timezone-naive" mode. Apache Arrow C GLib should support both of them.

### What changes are included in this PR?

This change adds a new `GTimeZone *time_zone` argument to `garrow_timestamp_data_type_new()` instead of adding a new `garrow_timestamp_data_type_new_time_zone()` function. This breaks backward compatibility for Apache Arrow C GLib users. But this still keeps backward compatibility for users of bindings such as Ruby and Vala. Because the new `GTimeZone *time_zone` is nullable.

I tried to use the "adding a new
`garrow_timestamp_data_type_new_time_zone()` function" approach but Vala didn't like it. Both of
`garrow_timestamp_data_type_new_time_zone()` (constructor) and `garrow_timestamp_data_type_get_time_zone()` (instance method or property reader) were mapped to
`GArrow.TimestampDataType.time_zone()`.

So I chose the "adding a new argument to
`garrow_timestamp_data_type_new()`" approach.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**

* Closes: apache#39702

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
…taType (apache#39717)

### Rationale for this change

Timestamp data type in Apache Arrow supports time zone but Apache Arrow C GLib didn't support it. Timestamp data type has "timezone-aware" mode and "timezone-naive" mode. If a timestamp data type has a valid time zone information, it uses "timezone-aware" mode. If a timestamp data type doesn't have a valid time zone information, it uses "timezone-naive" mode. Apache Arrow C GLib should support both of them.

### What changes are included in this PR?

This change adds a new `GTimeZone *time_zone` argument to `garrow_timestamp_data_type_new()` instead of adding a new `garrow_timestamp_data_type_new_time_zone()` function. This breaks backward compatibility for Apache Arrow C GLib users. But this still keeps backward compatibility for users of bindings such as Ruby and Vala. Because the new `GTimeZone *time_zone` is nullable.

I tried to use the "adding a new
`garrow_timestamp_data_type_new_time_zone()` function" approach but Vala didn't like it. Both of
`garrow_timestamp_data_type_new_time_zone()` (constructor) and `garrow_timestamp_data_type_get_time_zone()` (instance method or property reader) were mapped to
`GArrow.TimestampDataType.time_zone()`.

So I chose the "adding a new argument to
`garrow_timestamp_data_type_new()`" approach.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**

* Closes: apache#39702

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
…taType (apache#39717)

### Rationale for this change

Timestamp data type in Apache Arrow supports time zone but Apache Arrow C GLib didn't support it. Timestamp data type has "timezone-aware" mode and "timezone-naive" mode. If a timestamp data type has a valid time zone information, it uses "timezone-aware" mode. If a timestamp data type doesn't have a valid time zone information, it uses "timezone-naive" mode. Apache Arrow C GLib should support both of them.

### What changes are included in this PR?

This change adds a new `GTimeZone *time_zone` argument to `garrow_timestamp_data_type_new()` instead of adding a new `garrow_timestamp_data_type_new_time_zone()` function. This breaks backward compatibility for Apache Arrow C GLib users. But this still keeps backward compatibility for users of bindings such as Ruby and Vala. Because the new `GTimeZone *time_zone` is nullable.

I tried to use the "adding a new
`garrow_timestamp_data_type_new_time_zone()` function" approach but Vala didn't like it. Both of
`garrow_timestamp_data_type_new_time_zone()` (constructor) and `garrow_timestamp_data_type_get_time_zone()` (instance method or property reader) were mapped to
`GArrow.TimestampDataType.time_zone()`.

So I chose the "adding a new argument to
`garrow_timestamp_data_type_new()`" approach.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**

* Closes: apache#39702

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.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