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

Added section explaining legacy and new timestamp #12146

Closed
wants to merge 1 commit into from
Closed

Added section explaining legacy and new timestamp #12146

wants to merge 1 commit into from

Conversation

findepi
Copy link
Contributor

@findepi findepi commented Dec 28, 2018

No description provided.


.. note::

The new ``TIMESTAMP`` semantics is still experimental. It's recommended to keep
Copy link
Contributor

Choose a reason for hiding this comment

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

is -> are

Copy link
Contributor Author

Choose a reason for hiding this comment

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


The new ``TIMESTAMP`` semantics is still experimental. It's recommended to keep
the legacy ``TIMESTAMP`` semantics enabled. You can experiment with the new semantics
by enabling in the global configuration or session level. The legacy semantics
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid using "enable" here for the new semantics since the config property is inverted. How about

by configuring them globally or on a per-session basis

------------------

The legacy ``TIMESTAMP`` semantics can be set to ``true`` or ``false``
by setting ``deprecated.legacy-timestamp`` in the ``config.properties.``
Copy link
Contributor

Choose a reason for hiding this comment

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

In other documentation, like Properties Reference or release notes, we just say "config property" or "configuration property". Also, let's be more explicit about what true/false means.

The legacy semantics can be enabled using the ``deprecated.legacy-timestamp``
config property. Setting it to ``true`` (the default) enables the legacy semantics,
whereas setting it to ``false`` enables the new semantics.

The legacy ``TIMESTAMP`` semantics can be set to ``true`` or ``false``
by setting ``deprecated.legacy-timestamp`` in the ``config.properties.``

Additionally, there is a session level property ``legacy_timestamp``
Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, they can be enabled or disabled on per-session basis
with the ``legacy_timestamp`` session property.

Copy link
Contributor

Choose a reason for hiding this comment

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

small edit: on a per-session basis

by enabling in the global configuration or session level. The legacy semantics
may be deprecated in a future release.

Enabling/Disabling
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to "Configuration"

Previously, the ``TIMESTAMP`` type described an instance in time in the Presto session's time zone.
Now, Presto treats ``TIMESTAMP`` values as a set of the following fields representing wall time:

* ``YEAR OF ERA``
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify these to year, month, day, hour, minute, second?

I'm not sure that "of era", "of year", etc., add any value (it seems obvious what they mean, and I can't think of any other reasonable alternative interpretation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these terms follow terminology that can be found in Java Time and Joda Time.

  • java.time.LocalDateTime#getDayOfYear

  • java.time.LocalDateTime#getDayOfMonth

  • java.time.LocalDateTime#getDayOfWeek

  • org.joda.time.LocalDateTime#getDayOfYear

  • org.joda.time.LocalDateTime#getDayOfMonth

  • org.joda.time.LocalDateTime#getDayOfWeek

Joda LDT has 3 methods getYear*, but Java Time's LDT has just one getYear.

I can remove these OF .. from here. Please decide.

* ``MINUTE OF HOUR``
* ``SECOND OF MINUTE`` - as ``DECIMAL(5, 3)``

For that reason, a ``TIMESTAMP`` value is not linked with the session time zone in any way until a time zone is needed explicitly,
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines that wrap on GitHub are too long, please wrap to ~80 characters.

For that reason, when calculating the time zone offset for ``TIME WITH TIME ZONE``, Presto uses the session's start date and time.

This can be seen in queries using ``TIME WITH TIME ZONE`` in a time zone that has had time zone policy changes or uses DST.
eg. With session start time on 1 March 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use American rather than British formatting:

For example, with a session start time of March 1, 2017:

Or use ISO format:

For example, with a session start time of 2017-03-01:

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the session time zone matter for the example below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not. However, in legacy semantics, session time (instant when transaction started) matters.

TIME WITH TIME ZONE semantic changes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Due to compatibility requirements, having ``TIME WITH TIME ZONE`` completely aligned with the SQL standard was not possible yet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we explain what is different between the new Presto semantics and what is specified in the SQL standard?

Or is this really trying to say that it's not possible to follow the standard when using political time zones because the standard does not have them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter. Let's refactor this paragraph when we remove political zones from TIME.

@@ -133,6 +133,8 @@ String
Date and Time
-------------

Also see :doc:`/language/time`
Copy link
Contributor

Choose a reason for hiding this comment

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

We use "see also" in the docs. I picked that up from PostgreSQL, but it seems to be the proper way of writing citations.

@@ -7,3 +7,4 @@ SQL Language

language/types
language/reserved
language/time
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we name this "timestamp" rather than "time"?

@findepi
Copy link
Contributor Author

findepi commented Jan 15, 2019

@electrum thanks for review.
Comments applied. Can you clarify semantics is vs semantics are? I am following https://dictionary.cambridge.org/dictionary/english/semantics and using singular verb form.

@findepi findepi closed this Jan 24, 2019
@findepi findepi deleted the findepi/master/added-section-explaining-legacy-and-new-timestamp-e7fc54 branch January 24, 2019 15:52
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.

5 participants