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

feat(query): timestamp type can modify output format with new setting TIMESTAMP_OUTPUT_FORMAT #14174

Closed
wants to merge 3 commits into from

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Dec 27, 2023

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

timestamp type can modify output format with new setting TIMESTAMP_OUTPUT_FORMAT

About new setting

+-------------------------+-------------------------+-------------------------+----------------------------------------------------+---------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------+
| name                    | value                   | default                 | range                                              | level   | description                                                                                                                                   | type   |
+-------------------------+-------------------------+-------------------------+----------------------------------------------------+---------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------+
| timestamp_output_format | '%Y-%m-%d %H:%M:%S%.6f' | '%Y-%m-%d %H:%M:%S%.6f' | ["%Y-%m-%d %H:%M:%S%.3f", "%Y-%m-%d %H:%M:%S%.6f"] | SESSION | Specifies the display format for the TIMESTAMP data type alias. Available values include '%Y-%m-%d %H:%M:%S%.6f' and '%Y-%m-%d %H:%M:%S%.3f'. | String |
+-------------------------+-------------------------+-------------------------+----------------------------------------------------+---------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------+

Fixes #14169

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@TCeason TCeason changed the title feature(query): timestamp type can modify output format with new setting TIMESTAMP_OUTPUT_FORMAT feat(query): timestamp type can modify output format with new setting TIMESTAMP_OUTPUT_FORMAT Dec 27, 2023
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Dec 27, 2023
@TCeason TCeason force-pushed the add_ts_format branch 6 times, most recently from 85d55ef to 8d8dff8 Compare December 28, 2023 01:55
@TCeason
Copy link
Collaborator Author

TCeason commented Dec 28, 2023

@BohuTANG BohuTANG added the ci-cloud Build docker image for cloud test label Dec 28, 2023
Copy link
Contributor

Docker Image for PR

  • tag: pr-14174-bb779e3

note: this image tag is only available for internal use,
please check the internal doc for more details.

@youngsofun
Copy link
Member

youngsofun commented Dec 28, 2023

date/timestamp format should be part of file formats, not a setting.

  1. this is naturally part of a format spec, it should be stable.
  2. we do not hope users to rely on settings, most used internally (affect how we run it, but not the results).
  3. during a session, mutli diff date/timestamp formats may be involved

@TCeason
Copy link
Collaborator Author

TCeason commented Dec 28, 2023

date/timestamp format should be part of file formats, not a setting.

  1. this is naturally part of a format spec, it should be stable.
  2. we do not hope users to rely on settings, most used internally (affect how we run it, but not the results).
  3. during a session, mutli diff date/timestamp formats may be involved
  1. in this issue: Feature: Databend needs to distinguish between DateTime and Timestamp #14169 we need support format.
  2. sf does the same things. https://docs.snowflake.com/en/sql-reference/data-types-datetime#date-and-time-formats

@youngsofun
Copy link
Member

date/timestamp format should be part of file formats, not a setting.

  1. this is naturally part of a format spec, it should be stable.
  2. we do not hope users to rely on settings, most used internally (affect how we run it, but not the results).
  3. during a session, mutli diff date/timestamp formats may be involved
  1. in this issue: Feature: Databend needs to distinguish between DateTime and Timestamp #14169 we need support format.
  2. sf does the same things. https://docs.snowflake.com/en/sql-reference/data-types-datetime#date-and-time-formats

yes, like sf, we should distinguish query output and format output, it is ok for settings to only affect the former.
https://docs.snowflake.com/en/sql-reference/date-time-input-output#label-date-time-input-output-how-snowflake-determines
I think most of the time query output is interpreted to Date/TS type for PY/JAVA/.., instead of used directly, so the ts format for query output is not needed, it is only for transformation and presentation.

@BohuTANG
Copy link
Member

BohuTANG commented Dec 28, 2023

@youngsofun Yes, I think this is mainly for the third-party SaaS intergration. But now we can not say the superset side issue related to this timestamp format, @hantmac are still investigating.

pub fn timestamp_to_string(ts: i64, tz: Tz) -> impl Display {
ts.to_timestamp(tz).format(TIMESTAMP_FORMAT)
pub fn timestamp_to_string(ts: i64, tz: Tz, ts_format: &str) -> impl Display + '_ {
// 1. If write a wrong format, .format(ts_format) will display the ts_format directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid uninitialized function context:

Suggested change
// 1. If write a wrong format, .format(ts_format) will display the ts_format directly.
debug_assert!(!ts_format.trim().is_empty());
// 1. If write a wrong format, .format(ts_format) will display the ts_format directly.

@BohuTANG
Copy link
Member

Confirmed, there is no need add this setting, let's close this PR, thanks all.

@BohuTANG BohuTANG closed this Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cloud Build docker image for cloud test pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Databend needs to distinguish between DateTime and Timestamp
4 participants