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

Add timezone support to v3 SQL docs #5581

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

sanderson
Copy link
Collaborator

  • Updates information about time zones in SQL across all v3 products
  • Updates SQL data types documentation
  • Adds missing time and date functions
  • Adds information about how timezones change the behavior of some functions.

  • Rebased/mergeable

@telegraf-tiger
Copy link

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

| INTERVAL | INTERVAL | Interval(IntervalUnit::YearMonth) or Interval(IntervalUnit::DayTime) |
| SQL data type | Arrow data type | Description |
| :------------ | :----------------------------- | :------------------------------------------- |
| TIMESTAMP | Timestamp(Nanosecond, None) | Nanosecond timestamp with a time zone offset |
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a bit pedantic, but technically this is a timestamp without a time zone offset. This is what Arrow calls a "point in time".

I think the intention of what you have here is good though, so maybe change the Arrow data type to:
Timestamp(Nanosecond, Some("UTC")) which will also help solidify the change to UTC.

Comment on lines +77 to +85
'2024-01-01 00:00:00'::TIMESTAMP AT TIME ZONE 'America/Los_Angeles' AS 'Local time with TZ offset'
```

| UTC with TZ offset |
| :----------------------- |
| 2024-01-01T08:00:00.000Z |
| Local time with TZ offset |
| :------------------------ |
| 2024-01-01T00:00:00-08:00 |

{{% /expand %}}
{{< /expand-wrapper >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently suggesting this is a bug. Assuming others agree, this will get fixed in DF so that this behaves the same as the code above it.

Or, more accurately, this would be considered a bug in versions of 3.0 that have timezone support turned on.

| INTERVAL | INTERVAL | Interval(IntervalUnit::YearMonth) or Interval(IntervalUnit::DayTime) |
| SQL data type | Arrow data type | Description |
| :------------ | :----------------------------- | :------------------------------------------- |
| TIMESTAMP | Timestamp(Nanosecond, None) | Nanosecond timestamp with a time zone offset |
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

Comment on lines +77 to +85
'2024-01-01 00:00:00'::TIMESTAMP AT TIME ZONE 'America/Los_Angeles' AS 'Local time with TZ offset'
```

| UTC with TZ offset |
| :----------------------- |
| 2024-01-01T08:00:00.000Z |
| Local time with TZ offset |
| :------------------------ |
| 2024-01-01T00:00:00-08:00 |

{{% /expand %}}
{{< /expand-wrapper >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as below

| INTERVAL | INTERVAL | Interval(IntervalUnit::YearMonth) or Interval(IntervalUnit::DayTime) |
| SQL data type | Arrow data type | Description |
| :------------ | :----------------------------- | :------------------------------------------- |
| TIMESTAMP | Timestamp(Nanosecond, None) | Nanosecond timestamp with a time zone offset |
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Comment on lines +78 to +86
'2024-01-01 00:00:00'::TIMESTAMP AT TIME ZONE 'America/Los_Angeles' AS 'Local time with TZ offset'
```

| UTC with TZ offset |
| :----------------------- |
| 2024-01-01T08:00:00.000Z |
| Local time with TZ offset |
| :------------------------ |
| 2024-01-01T00:00:00-08:00 |

{{% /expand %}}
{{< /expand-wrapper >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as below


Converts a value to a date (`YYYY-MM-DD`).
Supports strings and numeric types as input.
Strings are parsed as `YYYY-MM-DD` unless another format is specified. if no Chrono formats are provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

Incomplete sentence.

content/influxdb/clustered/reference/sql/data-types.md Outdated Show resolved Hide resolved

Converts a value to a date (`YYYY-MM-DD`).
Supports strings and numeric types as input.
Strings are parsed as `YYYY-MM-DD` unless another format is specified. if no Chrono formats are provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

Incomplete sentence

@telegraf-tiger
Copy link

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger
Copy link

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

Copy link
Contributor

@jstirnaman jstirnaman left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe we need a note about the potential bug Jeff pointed out?

@sanderson
Copy link
Collaborator Author

@jeffreyssmith2nd I know this is an upstream bug. Are we going to hold off pushing the TZ support out until this bug is fixed? When should these doc changes go live?

@jeffreyssmith2nd
Copy link
Contributor

@jeffreyssmith2nd I know this is an upstream bug. Are we going to hold off pushing the TZ support out until this bug is fixed? When should these doc changes go live?

I don't know yet and am headed out for a long weekend. I'll check in with the team Tuesday on if we should delay or not.

Until then, I think we should hold the docs changes.


Converts a value to a date (`YYYY-MM-DD`).
Supports strings and numeric types as input.
Strings are parsed as `YYYY-MM-DD` unless another format is specified. if no Chrono formats are provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

Incomplete sentence

Strings are parsed as YYYY-MM-DD unless another format is specified. if no Chrono formats are provided.

?

@sanderson
Copy link
Collaborator Author

@jstirnaman I think you may have been viewing the diff of an old commit. I went through and fixed all of those.

@jeffreyssmith2nd
Copy link
Contributor

@sanderson Still on hold for the docs changes, waiting on some upstream discussion on if the ::timestamp thing is actually a bug or if it is intended behavior.

In the meantime, I'm removing all references to it in the docs where it isn't necessary to prevent any confusion for people down the line.

I will update the blog post and make a corresponding PR here as well once we know for sure what behavior we want out of ::timestamp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants